From 3170fc26abbdc82c8af891dfa968160408afcda2 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 13 Oct 2026 17:54:27 -0700 Subject: [PATCH 01/12] quest(ietf): claim object field passthrough Co-Authored-By: GPT-6 From 3a7a112ff85863393aeb75a53de94888379cb316 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 13 Oct 2026 19:51:16 -0700 Subject: [PATCH 02/12] feat(net): preserve IETF object fields through relays Co-Authored-By: GPT-6 --- doc/concept/standard.md | 13 +- quest/m1/ietf-extension/README.md | 13 +- quest/m1/ietf-extension/lite.md | 1 - quest/m1/ietf-extension/objects.md | 76 -- quest/m1/ietf-extension/subgroups.md | 6 +- quest/m1/ietf-extension/track-properties.md | 6 +- rs/moq-net/benches/group.rs | 42 +- rs/moq-net/src/fuzz.rs | 34 + rs/moq-net/src/ietf/datagram.rs | 4 +- rs/moq-net/src/ietf/group.rs | 55 +- rs/moq-net/src/ietf/mod.rs | 1 + rs/moq-net/src/ietf/object.rs | 201 ++++ rs/moq-net/src/ietf/publisher.rs | 571 ++++++---- rs/moq-net/src/ietf/subscriber.rs | 1103 +++++++++++++++---- rs/moq-net/src/lite/test_transport.rs | 17 +- rs/moq-net/src/model/datagram.rs | 28 + rs/moq-net/src/model/frame.rs | 42 +- rs/moq-net/src/model/group.rs | 236 +++- rs/moq-net/src/model/mod.rs | 3 +- rs/moq-net/src/model/object.rs | 61 + rs/moq-net/src/model/resume.rs | 83 +- rs/moq-net/src/model/track.rs | 92 +- 22 files changed, 2052 insertions(+), 636 deletions(-) delete mode 100644 quest/m1/ietf-extension/objects.md create mode 100644 rs/moq-net/src/ietf/object.rs create mode 100644 rs/moq-net/src/model/object.rs diff --git a/doc/concept/standard.md b/doc/concept/standard.md index 238f1f65ca2..fb09123a8de 100644 --- a/doc/concept/standard.md +++ b/doc/concept/standard.md @@ -26,12 +26,12 @@ maps everything else to "not supported" or a harmless equivalent. The subset drops. What a peer actually observes against this implementation: - **Pull.** Subscribers ask. Single-track `PUBLISH` offers are declined; announce a namespace and serve the resulting subscriptions. Announcements go out unsolicited, and we also ask for every prefix we may discover. The solicit `SETUP` option makes us wait to be asked. Without it, a draft 16+ peer also gets each match as a `NAMESPACE` on its `SUBSCRIBE_NAMESPACE` stream, so it hears the namespace twice. We never send `PUBLISH`: on drafts 16 and 17 a `SUBSCRIBE_NAMESPACE` asking only for `PUBLISH` is refused, one asking for both gets only `NAMESPACE`, and any other Subscribe Options value closes the session. -- **History is one group.** A `FETCH` returns one group from the cache, or on drafts 14 through 19 the saved prefix of the group a new subscription just joined. A range of groups is refused, as is a draft 20+ `LOCATION_FILTER` that ends at Largest Object rather than an absolute End Location. On draft 20 and later, `FETCH_OK`'s End Location is the requested end, capped at Largest Object: a range holding no objects gets an empty fetch stream, and only a start past Largest Object is refused, with `INVALID_RANGE`. A relay's copy with no upstream subscription can't know Largest Object short of the track's end, so it echoes the requested end instead. JavaScript publishing refuses every `FETCH`. Datagrams are never fetchable. A Rust reader that only fetches learns the track from `TRACK_STATUS` instead of subscribing, so a finished track stays fetchable; draft 17 still subscribes, since its `TRACK_STATUS` answer cannot say whether the track is timed, and so does a reader whose publisher refuses `TRACK_STATUS` as `NOT_SUPPORTED`. +- **History is one group.** In Rust, Object ID gaps must be marked by Prior Object ID Gap properties for a group to be fetchable. A `FETCH` returns one group from the cache, or on drafts 14 through 19 the saved prefix of the group a new subscription just joined. A range of groups is refused, as is a draft 20+ `LOCATION_FILTER` that ends at Largest Object rather than an absolute End Location. On draft 20 and later, `FETCH_OK`'s End Location is the requested end, capped at Largest Object: a range holding no objects gets an empty fetch stream, and only a start past Largest Object is refused, with `INVALID_RANGE`. A relay's copy with no upstream subscription can't know Largest Object short of the track's end, so it echoes the requested end instead. JavaScript publishing refuses every `FETCH`. Datagrams are never fetchable. A Rust reader that only fetches learns the track from `TRACK_STATUS` instead of subscribing, so a finished track stays fetchable; draft 17 still subscribes, since its `TRACK_STATUS` answer cannot say whether the track is timed, and so does a reader whose publisher refuses `TRACK_STATUS` as `NOT_SUPPORTED`. - **Timing.** A track whose `SUBSCRIBE_OK` declares no `TIMESCALE` is untimed, as is every track on drafts 14 through 16. A Rust reader that never subscribed takes the units from `TRACK_STATUS` on draft 18 and later. - **Strict SETUP and parameters.** Repeated unknown `SETUP` options, GREASE included, are accepted if well-formed; a repeated known option is rejected. On draft 17 and later, a `GROUP_ORDER` other than Ascending (1) or Descending (2) closes the session. Message parameters follow the negotiated draft's lists: drafts 14 and 15 ignore an unknown or misplaced parameter, draft 16 ignores one defined only for another message but closes the session on an unknown one, and draft 17 and later close it on both. - **One `SETUP` credential**, forwarded to the [auth server](/bin/relay/auth#the-contract) unverified. A second `SETUP` token closes the session. We advertise no token cache, so an alias reference in `SETUP` closes it with `PROTOCOL_VIOLATION`, one on a request with `UNKNOWN_AUTH_TOKEN_ALIAS`, and a registration on a request with `AUTH_TOKEN_CACHE_OVERFLOW`. A token attached to an individual request is otherwise decoded and grants nothing, and a request repeating one is refused with `MALFORMED_AUTH_TOKEN`. `SUBSCRIBE_TRACKS` is refused unread, so its token is not decoded. On draft 17 and later, the negotiated [MoQ Auth extension](/concept/moq-lite#authorization) accepts additional session tokens in band. - **Refused, not fatal.** A legal request this stack does not serve is rejected on its own and the session stays up: `FORWARD=0`, range filters, `SUBSCRIBE_TRACKS`, and the fetch forms above. On draft 19 and later, and in Rust on drafts 14 through 16, a subscription update may change only priority; any other update ends that subscription. -- **Datagrams** are a single normal object at object 0, forwarded without renumbering. Anything else is dropped, and a malformed one closes the session. Rust and JavaScript both carry them on every draft. +- **Objects and datagrams.** Rust relays preserve Object IDs, gaps, object properties, FIRST\_OBJECT, and END\_OF\_GROUP status objects between moq-transport peers for subgroup 0. Datagrams retain their IDs, properties, priority, and status. Plain Rust readers and moq-lite peers see consecutive frames and normal datagram payloads; JavaScript supports only normal datagrams at Object ID 0. A malformed datagram closes the session. Sparse groups cannot resume across route changes using frame ordinals; recovery refuses the group instead of requesting the wrong objects. - **Priority.** Higher is served first. The IETF default of 128 is this stack's 127, and a track that never sets one is 127. - **Size.** An object extension block larger than 64 KiB ends that subgroup stream. The session stays up. This cap is ours, not the draft's. @@ -61,12 +61,13 @@ product's model, not bugs, and the relay does not change them. requires the relay to send that `SUBSCRIBE` to all matching publishers. See [publisher epochs](/concept/moq-lite#publisher-epochs) for how that one route is chosen. -- **Unknown object properties are dropped.** +- **Plain readers leave object properties out.** [Draft 18 §2.5](https://www.ietf.org/archive/id/draft-ietf-moq-transport-18.html#section-2.5) says a relay that does not understand a property still forwards and caches - it. The model keeps a payload and a timestamp, and - [leaves other per-object metadata out](/concept/moq-lite#what-moq-lite-leaves-out), - so a property it does not understand stops at the session that delivered it. + it. Rust moq-transport relay paths do so; the public API, plain moq-lite, and + JavaScript [leave other per-object metadata out](/concept/moq-lite#what-moq-lite-leaves-out). + Immutable property blocks retain their bytes. Forwarding is refused when the + negotiated drafts use incompatible key or integer encodings. - **`SUBSCRIBE_OK` before an old source answers.** moq-lite 01 through 04 have no track stream, so the relay cannot learn from that source whether the track exists before answering. A moq-transport subscriber gets `SUBSCRIBE_OK` diff --git a/quest/m1/ietf-extension/README.md b/quest/m1/ietf-extension/README.md index 95c0850bc20..a4350a56e2a 100644 --- a/quest/m1/ietf-extension/README.md +++ b/quest/m1/ietf-extension/README.md @@ -28,11 +28,11 @@ Boundaries: Decided with the maintainer on 2026-10-13. This revisits the [#4926](https://github.com/moq-dev/moq/issues/4926) decline for relays only. That decline's cache-fragmentation concern does not apply here, because -an explicit `skip` tells the cache a gap is intentional, and a FETCH it +private Object IDs distinguish gaps from frame ordinals, and a FETCH it cannot answer unambiguously is refused. - **Carried:** subgroups with their publisher priority, object ID gaps as a - crate-private `skip: u64`, object properties as a decoded KVP list, + crate-private absolute Object IDs, object properties as a decoded KVP list, END_OF_GROUP status objects at their object ID, datagrams past object 0, and unknown track properties. - **Plain readers** see subgroup 0 only. A track with no subgroup 0 reads as @@ -52,9 +52,11 @@ cannot answer unambiguously is refused. moq-lite does not advertise this capability. - **Immutable properties** keep their serialized inner bytes and ingress encoding family, including across relay hops. Refuse forwarding between - absolute-key drafts 14/15 and delta-key drafts 16+ when immutable - properties are present; rewriting them would violate immutability and - forwarding them unchanged would change their meaning. + the absolute-key/QUIC-varint drafts 14/15, delta-key/QUIC-varint draft 16, + and delta-key/leading-ones-varint drafts 17+ when immutable properties + are present. Draft 17 is a separate family because it forbids 7-byte varints. + Rewriting them would violate immutability; forwarding unchanged + across those families would change their meaning. - **Public model carve-out.** These fields stay crate-private on purpose, an exception to [a public model](/quest/m1/model-public-surface.md). A third-party relay built on the public model drops them, as plain lite @@ -81,7 +83,6 @@ Work owned by this README, after the children land: ## Required -- [Object fields](/quest/m1/ietf-extension/objects.md) - object ID gaps, object properties, END_OF_GROUP status objects, and datagrams past object 0 survive moq-transport -> moq-relay -> moq-transport - [Subgroups](/quest/m1/ietf-extension/subgroups.md) - a group holds several subgroups, each forwarded on its own stream with its ID and exact publisher priority; plain readers see subgroup 0 - [Dynamic groups](/quest/m1/ietf-extension/dynamic-groups.md) - NEW_GROUP_REQUEST reaches the original publisher, coalesced across downstream subscribers - [Track properties](/quest/m1/ietf-extension/track-properties.md) - unknown track properties are forwarded unchanged in SUBSCRIBE_OK, FETCH_OK, and TRACK_STATUS_OK diff --git a/quest/m1/ietf-extension/lite.md b/quest/m1/ietf-extension/lite.md index 23920654b27..717a255edcd 100644 --- a/quest/m1/ietf-extension/lite.md +++ b/quest/m1/ietf-extension/lite.md @@ -57,7 +57,6 @@ and the framing above, only when negotiated. ## Required -- [Object fields](/quest/m1/ietf-extension/objects.md) - the per-object fields it frames - [Subgroups](/quest/m1/ietf-extension/subgroups.md) - the subgroup streams it frames - [Track properties](/quest/m1/ietf-extension/track-properties.md) - the track properties it frames - [Dynamic groups](/quest/m1/ietf-extension/dynamic-groups.md) - the upstream request signal and capability it carries diff --git a/quest/m1/ietf-extension/objects.md b/quest/m1/ietf-extension/objects.md deleted file mode 100644 index 2d9782e6829..00000000000 --- a/quest/m1/ietf-extension/objects.md +++ /dev/null @@ -1,76 +0,0 @@ -# [M] moq-transport object fields through a relay - -## Goal - -A moq-transport subscriber reading through moq-relay receives each object of -a single-subgroup track with the publisher's object ID, object properties, -and END_OF_GROUP status object. Datagrams past object 0 forward with their -object IDs, properties, and status instead of being dropped. Plain readers are -unchanged: frames in order with gaps invisible, and no properties. - -## Plan - -Decided 2026-10-13 (see the [epic](/quest/m1/ietf-extension/README.md)). - -Model, all crate-private in `rs/moq-net/src/model`: - -- Per frame: `skip: u64`, the object ID delta from the previous object minus - one, or the first object's ID. Also `properties`, a decoded KVP list - (`Property { id, value: Int(u64) | Bytes }` or similar). Keep a plain frame - small, for example an `Option>` for the rare extras, and extend the - model benchmark to show the cost. -- `frame::Info` does not change. Callers outside the crate build it with - struct literals, so a private field would break them. The fields go - through a crate-private writer on `group::Producer` and into internal - frame storage. -- Per group: the END_OF_GROUP status object's position, as a `skip` from the - last object. -- Keep the public, exhaustive `Datagram` unchanged so downstream struct - literals keep compiling. Store its object ID, properties, and END_OF_GROUP - status in a crate-private wrapper carried by internal track storage and - relay accessors; public writers use default metadata and public readers - receive only the original `Datagram`. - -IETF ingest and egress in `rs/moq-net/src/ietf`: - -- `next_object_id` stops refusing gaps. A stream that starts past object 0, - or with FIRST_OBJECT clear, is accepted as the subgroup's start when no fill - or resume is pending. Egress writes the stored delta and sets FIRST_OBJECT - from the stored state, instead of `slice.skip == 0`. -- Properties are decoded into the list, apart from those already mapped. The - timestamp still becomes `frame.timestamp`, and egress re-encodes the list - for the negotiated draft, because KVP encoding differs across drafts 14-22. - Immutable Properties (0x0B) keep their inner bytes verbatim. The relay adds - or removes nothing, because moq-test fails on a changed extension count. -- Preserve the ingress encoding family with immutable property bytes. - Refuse forwarding them between absolute-key drafts 14/15 and delta-key - drafts 16+, rather than rewriting authenticated bytes or changing their - meaning. The private lite carrier preserves this provenance too. -- An END_OF_GROUP status object is written where ingest saw one. Otherwise - egress ends the group with FIN and the header bit, as today. -- Datagrams past object 0 are mapped onto the same model, not dropped. -- FETCH serves a group only if every object ID jump in it is marked by a - Prior Object ID Gap property. Anything else gets `NOT_SUPPORTED`. FETCH - objects carry their properties. - -Plain readers (the public API, and lite egress without the extension) read -frames in order. Skip and properties never surface. - -Docs inline in `doc/concept/standard.md`: - -- The "Unknown object properties are dropped" deviation now applies only to - plain moq-lite and the public API. -- Update the datagram bullet. -- Gaps no longer refuse a group. - -Tests: a regression per feature through the IETF dispatch loop, moq-transport -publisher -> relay -> moq-transport subscriber. Cover draft 22 and one older -draft with a different KVP encoding. Shape the cases after moq-test: -increments of 2 and 3, a start object above 0, integer and bytes extensions, -an END_OF_GROUP marker, and datagrams in mode 3. -Also cover immutable byte preservation within an encoding family and -refusal across the draft-15/16 boundary in both directions, using a nested -list with multiple property types. - -Public API: none. Wire: none new; moq-transport fields that were dropped are -now forwarded. diff --git a/quest/m1/ietf-extension/subgroups.md b/quest/m1/ietf-extension/subgroups.md index 7792bb67e5f..391fc1cd930 100644 --- a/quest/m1/ietf-extension/subgroups.md +++ b/quest/m1/ietf-extension/subgroups.md @@ -13,7 +13,7 @@ Decided 2026-10-13 (see the [epic](/quest/m1/ietf-extension/README.md)). - `group::Producer` and `group::Consumer` own crate-private subgroups. Subgroup 0 is the public frame list. Every other subgroup keeps its own frames with - their `skip`, properties, end status, and priority. + their Object IDs, properties, end status, and priority. - Priority is stored as the wire's `Option`. `None` means the header omitted it and inherits the track default. It round-trips exactly, so 200 stays 200 and never goes through the 128-to-127 track mapping. @@ -37,7 +37,3 @@ benchmark swept over subgroups per group and subscribers, since subgroups fan out. Public API: none. Wire: none new. - -## Required - -- [Object fields](/quest/m1/ietf-extension/objects.md) - subgroups store the per-object skip, properties, and end status it adds diff --git a/quest/m1/ietf-extension/track-properties.md b/quest/m1/ietf-extension/track-properties.md index b9443160d24..a2987e4065f 100644 --- a/quest/m1/ietf-extension/track-properties.md +++ b/quest/m1/ietf-extension/track-properties.md @@ -12,9 +12,8 @@ handled as today. Decided 2026-10-13 (see the [epic](/quest/m1/ietf-extension/README.md)). - Store the unknown properties crate-private on `track::Info`, which is - `#[non_exhaustive]`, using the `Property` list from - [object fields](/quest/m1/ietf-extension/objects.md). Re-encode them for each - egress draft. Immutable Properties keep their inner bytes verbatim and + `#[non_exhaustive]`, using the private `model::object::Property` list. + Re-encode them for each egress draft. Immutable Properties keep their inner bytes verbatim and their ingress encoding family; the cross-family refusal from object fields applies to track properties too. - Unknown properties include those the draft defines but moq-net does not @@ -35,5 +34,4 @@ Public API: none. Wire: none new. ## Required -- [Object fields](/quest/m1/ietf-extension/objects.md) - introduces the decoded `Property` list - [Dynamic groups](/quest/m1/ietf-extension/dynamic-groups.md) - makes DYNAMIC_GROUPS advertising truthful by carrying NEW_GROUP_REQUEST upstream diff --git a/rs/moq-net/benches/group.rs b/rs/moq-net/benches/group.rs index 13df04e69af..b8ca1fb0c1b 100644 --- a/rs/moq-net/benches/group.rs +++ b/rs/moq-net/benches/group.rs @@ -319,5 +319,45 @@ fn bench_roundtrip(c: &mut Criterion) { g.finish(); } -criterion_group!(benches, bench_write, bench_read, bench_roundtrip, bench_track_recv); +/// Sweep payload depth and subscribers for both plain and property-bearing frames. +fn bench_relay_frames(c: &mut Criterion) { + #[cfg(feature = "fuzz")] + { + let mut g = c.benchmark_group("group_relay_frames"); + for frames in [4, 512, 8192] { + for subscribers in [1, 8, 64] { + for properties in [false, true] { + let context = moq_net::fuzz::object_group(frames, properties); + let mode = if properties { "properties" } else { "plain" }; + g.throughput(Throughput::Elements((frames * subscribers) as u64)); + g.bench_function(BenchmarkId::new(mode, format!("{frames}/{subscribers}")), |b| { + b.iter_batched( + || (0..subscribers).map(|_| context.2.consume()).collect::>(), + |mut readers| { + let mut buffer = frame::Buffer::<8>::new(); + for reader in &mut readers { + drain_batched(reader, &mut buffer); + } + readers + }, + BatchSize::SmallInput, + ); + }); + } + } + } + g.finish(); + } + #[cfg(not(feature = "fuzz"))] + let _ = c; +} + +criterion_group!( + benches, + bench_write, + bench_read, + bench_roundtrip, + bench_track_recv, + bench_relay_frames +); criterion_main!(benches); diff --git a/rs/moq-net/src/fuzz.rs b/rs/moq-net/src/fuzz.rs index 9f40914cd24..bb068cb397a 100644 --- a/rs/moq-net/src/fuzz.rs +++ b/rs/moq-net/src/fuzz.rs @@ -24,6 +24,40 @@ use crate::{ /// [`seeds`] is checked against. pub type Target = fn(&[u8]) -> bool; +/// Build a cached group with optional private object fields for model benchmarks. +pub fn object_group(frames: usize, properties: bool) -> (broadcast::Producer, track::Producer, group::Producer) { + let broadcast = broadcast::Producer::new(broadcast::Info::default()); + let track = broadcast.create_track("objects", None).unwrap(); + let mut group = track.append_group().unwrap(); + let payload = Bytes::from_static(&[0; 64]); + for id in 0..frames { + let fields = properties.then(|| { + std::sync::Arc::new(crate::model::object::Fields { + id: id as u64, + properties: crate::model::object::Properties { + values: vec![crate::model::object::Property { + id: 0x80, + value: crate::model::object::Value::Int(96_000), + }], + ..Default::default() + }, + }) + }); + let header = frame::Header { + info: frame::Info { + size: payload.len() as u64, + timestamp: None, + }, + fields, + }; + let mut frame = group.create_frame_owned(header, &frame::Budget::default()).unwrap(); + frame.write(payload.clone()).unwrap(); + frame.finish().unwrap(); + } + group.finish().unwrap(); + (broadcast, track, group) +} + /// Every target, keyed by the name of its `fuzz_targets/.rs` shim. pub const TARGETS: &[(&str, Target)] = &[ ("lite", lite_wire), diff --git a/rs/moq-net/src/ietf/datagram.rs b/rs/moq-net/src/ietf/datagram.rs index ceec18bd21a..008d7e931f4 100644 --- a/rs/moq-net/src/ietf/datagram.rs +++ b/rs/moq-net/src/ietf/datagram.rs @@ -1,8 +1,8 @@ //! OBJECT_DATAGRAM: one Object carried in a QUIC datagram (draft-14 section 10.3.1 through //! draft-20 section 11.3.1). //! -//! The model counterpart is [`crate::Datagram`], a single-frame group, so only an Object at -//! ID 0 maps onto it. The Type is a set of flags on every draft; draft-14 lacks the +//! Public [`crate::Datagram`] writers emit Object ID 0; private relay metadata preserves +//! other IDs, properties, and status. The Type is a set of flags; draft-14 lacks the //! DEFAULT_PRIORITY bit and a status with an omitted Object ID. use bytes::Bytes; diff --git a/rs/moq-net/src/ietf/group.rs b/rs/moq-net/src/ietf/group.rs index 7eb58215e19..4191a287ec2 100644 --- a/rs/moq-net/src/ietf/group.rs +++ b/rs/moq-net/src/ietf/group.rs @@ -18,6 +18,7 @@ const PROP_TIMESTAMP: u64 = 0x10; /// Draft-03's body text and its IANA table disagreed (0x0A vs 0x06); this is the /// table's value, which is what we and other implementations shipped. Draft-04 /// assigns 0x0A to Secure Objects private properties, so it is not accepted here. +#[cfg(test)] const PROP_TIMESTAMP_DRAFT03: u64 = 0x06; /// Implementation limit for an object's extension (property) block, independent of the @@ -72,48 +73,20 @@ fn encode_object_property_type(w: &mut Encoder<'_>, kind: u64, prev: u64, versio w.varint(encoded) } -/// Decode the Timestamp (0x10) Object Property from an object's extension block, -/// skipping any other properties. Returns `None` when no Timestamp property is present. -/// -/// `timescale` is the track's declared units. An object-scope Timescale (0x08), which -/// draft-ietf-moq-loc-04 permits, is ignored: timedness and units are per track, so the -/// track's TIMESCALE alone decides them. +/// Decode the same property list as ingest when checking timestamps in wire tests. +#[cfg(test)] pub fn decode_object_time( r: &mut Decoder<'_>, timescale: Timescale, version: Version, ) -> Result, DecodeError> { - let mut timestamp: Option = None; - let mut prev_type: u64 = 0; - let mut first = true; - - while !r.is_empty() { - let step = r.varint()?; - let abs = match version { - Version::Draft14 | Version::Draft15 => step, - _ if first => step, - _ => prev_type.checked_add(step).ok_or(DecodeError::BoundsExceeded)?, - }; - first = false; - prev_type = abs; - - if abs % 2 == 0 { - // Even type: a single varint value. - let value = r.varint()?; - match abs { - PROP_TIMESTAMP | PROP_TIMESTAMP_DRAFT03 => timestamp = Some(value), - _ => {} - } - } else { - // Odd type: length-prefixed bytes we don't care about. - let len = usize::try_from(r.varint()?).map_err(|_| DecodeError::BoundsExceeded)?; - r.slice(len)?; - } - } - - timestamp - .map(|value| Timestamp::new(value, timescale).map_err(|_| DecodeError::InvalidValue)) - .transpose() + let data = bytes::Bytes::copy_from_slice(r.rest()); + super::object::read(&data, version, Some(timescale)) + .map(|decoded| decoded.timestamp) + .map_err(|error| match error { + crate::Error::Decode(error) => error, + _ => DecodeError::InvalidValue, + }) } #[derive(Debug, Clone, Copy, PartialEq, Eq, TryFromPrimitive, IntoPrimitive)] @@ -207,9 +180,8 @@ pub struct GroupFlags { /// Whether this stream carries the subgroup from its first published object. /// - /// A subgroup that starts partway through has a hole at the front, which moq-lite cannot - /// represent. Always true on the drafts that predate the FIRST_OBJECT bit (before - /// draft-18), where there is no such signal to read. + /// Independent of its first Object ID: a publisher can start a subgroup above 0. + /// Always true before draft-18, where there is no such signal to read. pub first_object: bool, } @@ -625,8 +597,7 @@ mod tests { } /// Draft-18 introduces the FIRST_OBJECT bit (0x40) per spec §11.4.2. - /// moq-lite always sets it on emit and ignores it on decode (we already - /// require what the bit asserts). + /// Plain groups set it; relayed groups preserve the publisher's claim. #[test] fn test_first_object_bit_draft18() { // Encoding sets bit 0x40 for default flags. diff --git a/rs/moq-net/src/ietf/mod.rs b/rs/moq-net/src/ietf/mod.rs index aaf83a76562..1834293bb22 100644 --- a/rs/moq-net/src/ietf/mod.rs +++ b/rs/moq-net/src/ietf/mod.rs @@ -26,6 +26,7 @@ pub mod hidden; mod location; pub mod message; mod namespace; +mod object; pub mod peer; mod priority; mod properties; diff --git a/rs/moq-net/src/ietf/object.rs b/rs/moq-net/src/ietf/object.rs new file mode 100644 index 00000000000..a1067097139 --- /dev/null +++ b/rs/moq-net/src/ietf/object.rs @@ -0,0 +1,201 @@ +use bytes::Bytes; + +use super::Version; +use crate::{ + Error, Timescale, Timestamp, + coding::{Decoder, Encoder}, + model::object::{Encoding, Properties, Property, Value}, +}; + +pub(super) fn encoding(version: Version) -> Encoding { + match version { + Version::Draft14 | Version::Draft15 => Encoding::Absolute, + Version::Draft16 => Encoding::DeltaQuic, + Version::Draft17 => Encoding::Delta { seven: false }, + _ => Encoding::Delta { seven: true }, + } +} + +pub(super) struct Decoded { + pub properties: Properties, + pub timestamp: Option, + pub gap: Option, +} + +struct List { + properties: Properties, + time: Option, + gap: Option, +} + +fn list(data: &Bytes, version: Version, immutable: bool) -> Result { + let keys = encoding(version); + let mut decoder = Decoder::new(data, version.into()); + let mut result = List { + properties: Properties::default(), + time: None, + gap: None, + }; + let mut previous = 0u64; + while !decoder.is_empty() { + let key = decoder.varint()?; + let id = match keys { + Encoding::Absolute => key, + Encoding::Delta { .. } | Encoding::DeltaQuic => previous.checked_add(key).ok_or(Error::MalformedTrack)?, + }; + previous = id; + let value = if id % 2 == 0 { + let value = decoder.varint()?; + if matches!(id, 0x10 | 0x06) { + result.time = Some(value); + } + if id == 0x3e && result.gap.replace(value).is_some() { + return Err(Error::MalformedTrack); + } + if id == 0x10 { + if result.properties.timestamp.replace(value).is_some() { + return Err(Error::MalformedTrack); + } + continue; + } + Value::Int(value) + } else { + let bytes = decoder.bytes()?; + if bytes.len() > u16::MAX as usize { + return Err(Error::MalformedTrack); + } + // A small cached property must not pin the reader's larger receive buffer. + let bytes = Bytes::copy_from_slice(bytes); + if id == 0x0b { + if immutable || result.properties.immutable.replace(keys).is_some() { + return Err(Error::MalformedTrack); + } + let nested = list(&bytes, version, true).map_err(|_| Error::MalformedTrack)?; + result.time = result.time.or(nested.time); + if let Some(gap) = nested.gap + && result.gap.replace(gap).is_some() + { + return Err(Error::MalformedTrack); + } + } + Value::Bytes(bytes) + }; + result.properties.values.push(Property { id, value }); + } + Ok(result) +} + +pub(super) fn read(data: &Bytes, version: Version, timescale: Option) -> Result { + let parsed = list(data, version, false)?; + let timestamp = match (parsed.time, timescale) { + (Some(value), Some(timescale)) => Some(Timestamp::new(value, timescale).map_err(|_| Error::MalformedTrack)?), + _ => None, + }; + Ok(Decoded { + properties: parsed.properties, + timestamp, + gap: parsed.gap, + }) +} + +pub(super) fn write(properties: &Properties, version: Version) -> Result, Error> { + if properties + .immutable + .is_some_and(|original| original != encoding(version)) + { + return Err(Error::Unsupported); + } + let timestamp = properties.timestamp.map(|value| Property { + id: 0x10, + value: Value::Int(value), + }); + let mut sorted: Vec<_> = properties.values.iter().chain(timestamp.as_ref()).collect(); + // Delta keys require ascending order. Equal keys retain the publisher's order. + sorted.sort_by_key(|property| property.id); + let mut out = Vec::new(); + let mut encoder = Encoder::new(&mut out, version.into()); + let mut previous = 0; + for property in sorted { + encoder.varint(match encoding(version) { + Encoding::Absolute => property.id, + Encoding::Delta { .. } | Encoding::DeltaQuic => property.id - previous, + })?; + previous = property.id; + match &property.value { + Value::Int(value) => encoder.varint(*value)?, + Value::Bytes(value) => encoder.bytes(value)?, + } + } + Ok(out) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn cached_bytes_do_not_retain_the_receive_buffer() { + use std::sync::{ + Arc, + atomic::{AtomicBool, Ordering}, + }; + struct Buffer { + bytes: Vec, + dropped: Arc, + } + impl AsRef<[u8]> for Buffer { + fn as_ref(&self) -> &[u8] { + &self.bytes + } + } + impl Drop for Buffer { + fn drop(&mut self) { + self.dropped.store(true, Ordering::Relaxed); + } + } + let dropped = Arc::new(AtomicBool::new(false)); + let mut bytes = vec![0; 65536]; + bytes[..3].copy_from_slice(&[5, 1, 42]); + let received = Bytes::from_owner(Buffer { + bytes, + dropped: dropped.clone(), + }) + .slice(..3); + let decoded = read(&received, Version::Draft22, None).unwrap(); + drop(received); + assert!(dropped.load(Ordering::Relaxed)); + assert_eq!( + decoded.properties.values[0].value, + Value::Bytes(Bytes::from_static(&[42])) + ); + } + + #[test] + fn immutable_timestamp_is_read_without_adding_an_outer_property() { + let bytes = Bytes::from_static(&[0x0b, 2, 0x10, 42]); + let decoded = read(&bytes, Version::Draft22, Some(Timescale::MILLI)).unwrap(); + assert_eq!(decoded.timestamp, Some(Timestamp::from_millis(42).unwrap())); + assert_eq!(decoded.properties.timestamp, None); + assert_eq!(write(&decoded.properties, Version::Draft18).unwrap(), bytes); + assert!(matches!( + write(&decoded.properties, Version::Draft17), + Err(Error::Unsupported) + )); + } + + #[test] + fn immutable_lists_validate_nested_blocks_and_integer_forms() { + for bytes in [&[0x0b, 2, 0x0b, 0][..], &[0x0b, 0, 0, 0][..], &[0x0b, 1, 2][..]] { + assert!(read(&Bytes::copy_from_slice(bytes), Version::Draft22, None).is_err()); + } + // Draft-18 permits this nonminimal seven-byte value; draft-17 cannot read it. + let bytes = Bytes::from_static(&[0x0b, 8, 2, 0xfc, 0, 0, 0, 0, 0, 1]); + let decoded = read(&bytes, Version::Draft18, None).unwrap(); + assert_eq!(write(&decoded.properties, Version::Draft22).unwrap(), bytes); + assert!(read(&bytes, Version::Draft17, None).is_err()); + assert!(matches!( + write(&decoded.properties, Version::Draft17), + Err(Error::Unsupported) + )); + } +} diff --git a/rs/moq-net/src/ietf/publisher.rs b/rs/moq-net/src/ietf/publisher.rs index 56a3045694c..9d1175c734d 100644 --- a/rs/moq-net/src/ietf/publisher.rs +++ b/rs/moq-net/src/ietf/publisher.rs @@ -70,7 +70,7 @@ enum FetchPrior { /// The first object on the stream. None, /// The next object of the same group. - Same, + Same(Option), } /// The group a FETCH answers with, read out of the cache so a later eviction cannot @@ -79,27 +79,48 @@ struct FetchedGroup { sequence: u64, /// The Object ID of the first frame. first: u64, - frames: Vec, + frames: Vec, /// The fetched range reached the finished group's final object. complete: bool, /// The track's info, which FETCH_OK describes and whose units stamp the objects. info: track::Info, } -impl FetchPrior { - /// The prior for the next object of a single-group stream, clearing `first`. - fn next(first: &mut bool) -> Self { - match std::mem::take(first) { - true => Self::None, - false => Self::Same, - } - } +struct FetchObjectInfo<'a> { + sequence: u64, + object: u64, + prior: FetchPrior, + timestamp: Option, + timescale: Option, + version: Version, + fields: Option<&'a crate::model::object::Fields>, +} + +fn fetch_prior(version: Version, prior: Option, object: u64) -> Result { + let Some(prior) = prior else { + return Ok(FetchPrior::None); + }; + let step = object + .checked_sub(prior) + .filter(|step| *step > 0) + .ok_or(Error::MalformedTrack)?; + Ok(FetchPrior::Same((step != 1).then_some(match version { + Version::Draft14 | Version::Draft15 | Version::Draft16 | Version::Draft17 => object, + _ => step, + }))) } impl FetchedGroup { /// One past the last Object ID read. fn end(&self) -> u64 { - self.first + self.frames.len() as u64 + self.frames.last().map_or(self.first, |frame| { + frame + .fields + .as_ref() + .map_or(self.first + self.frames.len() as u64, |fields| { + fields.id.saturating_add(1) + }) + }) } } @@ -116,30 +137,58 @@ async fn read_fetch( priority: u8, ) -> Result { let info = track.query().await?; - let fetch = group::Fetch { - priority, - frame_start: skip, + let cached = track.peek_group(sequence); + let wire = cached.as_ref().and_then(|group| group.wire()); + if wire.as_ref().is_some_and(|wire| !wire.fetchable) { + return Err(Error::Unsupported); + } + let frame_start = match (&wire, &cached) { + (Some(wire), Some(group)) if skip >= wire.first_id => group.index(), + _ => skip, }; - let mut group = track.fetch_group(sequence, fetch).await?; - - // `fetch_group` positions the consumer at `skip`, or refuses a group that no - // longer holds it. - let first = group.index(); + let mut group = track + .fetch_group(sequence, group::Fetch { priority, frame_start }) + .await?; + let mut first = skip; let mut frames = Vec::new(); + // Reading an object past the range proves the range missed the group's end. + let mut overshot = false; // A long cached group is read a slice per poll, so the session's other tasks still run. let mut budget = kio::coop::Budget::new(32); - while until.is_none_or(|until| first + (frames.len() as u64) < until) { + loop { kio::wait(|waiter| budget.poll_yield(waiter)).await; - let Some(frame) = group.read_frame().await? else { + if group.wire().is_some_and(|wire| !wire.fetchable) { + return Err(Error::Unsupported); + } + let ordinal = group.index(); + let Some(frame) = kio::wait(|waiter| group.poll_read_object(waiter)).await? else { break; }; + let id = frame.fields.as_ref().map_or(ordinal, |fields| fields.id); + if until.is_some_and(|until| id >= until) { + overshot = true; + break; + } + if id < skip { + continue; + } + if frames.is_empty() { + first = id; + } frames.push(frame); + if until.is_some_and(|until| id.saturating_add(1) >= until) { + break; + } } // A bounded range can reach the group's end without reading its terminal None, and a // start past the end is already there. - let complete = - matches!(group.finished().now_or_never(), Some(Ok(_))) && group.index() >= group.frame_count() as u64; + let complete = !overshot + && matches!(group.finished().now_or_never(), Some(Ok(_))) + && group.index() >= group.frame_count() as u64; + if group.wire().is_some_and(|wire| !wire.fetchable) { + return Err(Error::Unsupported); + } Ok(FetchedGroup { sequence, first, @@ -1264,9 +1313,12 @@ where version: Version, ) -> Result<(), Error> { let mut index: u64 = 0; - let mut first = true; + let mut prior = None; - let mut buf: frame::Buffer = frame::Buffer::new(); + let mut buf = arrayvec::ArrayVec::::new(); + if group.wire().is_some_and(|wire| !wire.fetchable) { + return Err(Error::Unsupported); + } // A long cached group is written a slice per poll, so the session's other tasks still run. let mut budget = kio::coop::Budget::new(32); 'serve: loop { @@ -1283,7 +1335,7 @@ where if waiter.poll_future(closed.as_mut()).is_ready() { return Poll::Ready(Err(Error::Cancel)); } - match group.poll_read_frames(waiter, &mut buf) { + match group.poll_read_objects(waiter, &mut buf) { Poll::Pending => match group.poll_next_frame(waiter) { Poll::Ready(Ok(Some(frame))) => Poll::Ready(Ok(FillStep::Partial(frame))), Poll::Ready(Ok(None)) => Poll::Ready(Ok(FillStep::Done)), @@ -1300,29 +1352,41 @@ where match step? { FillStep::Batch => { - for i in 0..buf.filled().len() { + for frame in &buf { + if until.is_some_and(|until| index >= until) { + break 'serve; + } + let frame = frame.clone(); + index = frame.fields.as_ref().map_or(index, |fields| fields.id); if until.is_some_and(|until| index >= until) { break 'serve; } - let frame = buf.filled()[i].clone(); if index >= skip { Self::write_fetch_object( stream, - sequence, - index, - FetchPrior::next(&mut first), - frame.timestamp, - timescale, - version, + FetchObjectInfo { + sequence, + object: index, + prior: fetch_prior(version, prior, index)?, + timestamp: frame.timestamp, + timescale, + version, + fields: frame.fields.as_deref(), + }, ) .await?; - Self::write_fetch_payload(stream, frame.payload, version).await?; + Self::write_fetch_payload(stream, frame.frame.payload, version).await?; + prior = Some(index); } index += 1; group.keep_alive(); } } FillStep::Partial(mut frame) => { + index = frame.fields.as_ref().map_or(index, |fields| fields.id); + if until.is_some_and(|until| index >= until) { + break; + } if index < skip { // A skipped frame still has to be drained to advance the cursor. loop { @@ -1346,14 +1410,18 @@ where Self::write_fetch_object( stream, - sequence, - index, - FetchPrior::next(&mut first), - frame.timestamp, - timescale, - version, + FetchObjectInfo { + sequence, + object: index, + prior: fetch_prior(version, prior, index)?, + timestamp: frame.timestamp, + timescale, + version, + fields: frame.fields.as_deref(), + }, ) .await?; + prior = Some(index); index += 1; stream.varint(frame.size).await?; @@ -1392,31 +1460,39 @@ where /// Write one fetch object's header and timestamp properties in the negotiated layout. async fn write_fetch_object( stream: &mut Writer, Version>, - sequence: u64, - object: u64, - prior: FetchPrior, - timestamp: Option, - timescale: Option, - version: Version, + header: FetchObjectInfo<'_>, ) -> Result<(), Error> { - // An unstamped object has no properties at all, so the field is omitted rather - // than written empty: the track declared no units to read a timestamp in. - // Drafts 14-16 never declare TIMESCALE, so they never stamp either. + let FetchObjectInfo { + sequence, + object, + prior, + timestamp, + timescale, + version, + fields, + } = header; + // Relayed properties survive without a timescale. Plain frames generate a + // timestamp only on drafts that can declare its units. let timescale = timescale.filter(|_| ietf::Properties::sends_timescale(version)); - let properties = match timescale { - Some(timescale) => { - // Every frame on a timed track is timed (see `track::Info::timescale`). - let timestamp = timestamp.ok_or(Error::TimestampMismatch)?; - let mut properties = Vec::new(); - ietf::encode_object_time( - &mut Encoder::new(&mut properties, version.into()), - timestamp, - timescale, - version, - )?; - Some(properties) + let properties = if let Some(fields) = fields { + let properties = super::object::write(&fields.properties, version)?; + (!properties.is_empty()).then_some(properties) + } else { + match timescale { + Some(timescale) => { + // Every frame on a timed track is timed (see `track::Info::timescale`). + let timestamp = timestamp.ok_or(Error::TimestampMismatch)?; + let mut properties = Vec::new(); + ietf::encode_object_time( + &mut Encoder::new(&mut properties, version.into()), + timestamp, + timescale, + version, + )?; + Some(properties) + } + None => None, } - None => None, }; if version == Version::Draft14 { @@ -1443,10 +1519,10 @@ where properties, }, // Same group and priority; the Object ID is the prior one plus one. - FetchPrior::Same => ietf::FetchObject::Object { + FetchPrior::Same(delta) => ietf::FetchObject::Object { subgroup: ietf::FetchSubgroup::Zero, group: None, - object: None, + object: delta, priority: None, properties, }, @@ -1694,6 +1770,13 @@ where Some(Err(err)) => return self.reject_fetch(stream, msg.request_id, &err, &err.to_string()).await, None => return Ok(()), }; + for frame in &group.frames { + if let Some(fields) = &frame.fields + && let Err(err) = super::object::write(&fields.properties, self.version) + { + return self.reject_fetch(stream, msg.request_id, &err, &err.to_string()).await; + } + } let draft20 = Filter::is_draft20(self.version); let end_of_track = !joined && group.complete && track.final_sequence() == group.sequence.checked_add(1); // Draft-20 caps the response at Largest Object, and refuses a start past it (section @@ -1811,19 +1894,25 @@ where request_id: msg.request_id, }) .await?; - let mut first = true; - for (object, frame) in (group.first..).zip(group.frames) { + let mut prior = None; + for (ordinal, frame) in (group.first..).zip(group.frames) { + let object = frame.fields.as_ref().map_or(ordinal, |fields| fields.id); + let position = fetch_prior(self.version, prior, object)?; + prior = Some(object); Self::write_fetch_object( &mut writer, - group.sequence, - object, - FetchPrior::next(&mut first), - frame.timestamp, - timescale, - self.version, + FetchObjectInfo { + sequence: group.sequence, + object, + prior: position, + timestamp: frame.timestamp, + timescale, + version: self.version, + fields: frame.fields.as_deref(), + }, ) .await?; - Self::write_fetch_payload(&mut writer, frame.payload, self.version).await?; + Self::write_fetch_payload(&mut writer, frame.frame.payload, self.version).await?; } settle(&mut writer).await?; Ok::<(), Error>(()) @@ -3115,6 +3204,7 @@ impl TrackServe { continue; } + let wire = group.wire(); let msg = ietf::GroupHeader { track_alias: self.request_id.0, group_id: sequence, @@ -3127,11 +3217,14 @@ impl TrackServe { // Object Property) only when TIMESCALE was declared for this version. // Drafts 14-16 cannot, so the flag stays clear. flags: ietf::GroupFlags { - has_extensions: self.timescale.is_some(), - first_object: slice.skip == 0, + has_extensions: self.timescale.is_some() + || wire.as_ref().is_some_and(|wire| wire.extensions), + first_object: wire + .as_ref() + .map_or(slice.skip == 0, |wire| wire.first_object && slice.skip <= wire.first_id), // A stream capped by the range stops before the group may end, so its // FIN cannot claim END_OF_GROUP. - has_end: slice.until.is_none(), + has_end: slice.until.is_none() && wire.as_ref().is_none_or(|wire| wire.has_end), ..Default::default() }, }; @@ -3156,7 +3249,7 @@ impl TrackServe { } Poll::Ready(Ok(None)) => { // Datagrams written before the track finished still go out. - self.poll_datagrams(waiter); + self.poll_datagrams(waiter)?; self.draining = true; if let Poll::Ready(Ok(end)) = self.track.poll_finished(waiter) { self.end = Some(end); @@ -3168,7 +3261,7 @@ impl TrackServe { } } // Groups first, so a burst of datagrams cannot starve them. - self.poll_datagrams(waiter); + self.poll_datagrams(waiter)?; // Newly created group machines start now rather than on the next wake. let _ = self.children.poll(waiter); Poll::Pending @@ -3176,42 +3269,81 @@ impl TrackServe { /// Send every buffered datagram as an OBJECT_DATAGRAM, best-effort, like moq-lite. /// - /// A datagram is Object 0 of a group that has no other, so the Object ends its group. + /// Public datagrams are Object 0 and end their group; relayed ones retain their fields. /// The track's end or failure surfaces through its groups, so this only stops. - fn poll_datagrams(&mut self, waiter: &kio::Waiter) { + fn poll_datagrams(&mut self, waiter: &kio::Waiter) -> Result<(), Error> { if !self.datagrams { - return; + return Ok(()); } - while let Poll::Ready(Ok(Some(datagram))) = self.track.poll_recv_datagram(waiter) { + while let Poll::Ready(Ok(Some(datagram))) = self.track.poll_recv_wire_datagram(waiter) { let sequence = datagram.sequence; - let properties = match self.timescale { - Some(timescale) => { - // Every datagram on a timed track is timed (see `track::Info::timescale`). - let Some(timestamp) = datagram.timestamp else { - continue; + let id = datagram.wire.as_ref().and_then(|wire| wire.object_id).unwrap_or(0); + if self + .range + .start + .is_some_and(|start| start.group == sequence && id < start.object) + || self + .range + .end + .is_some_and(|end| end.group == sequence && end.object.is_some_and(|end| id > end)) + { + continue; + } + let (object_id, priority, end_of_group, status, properties) = match &datagram.wire { + Some(wire) => { + if wire.status == Some(1) && !matches!(self.version, Version::Draft14 | Version::Draft15) { + return Err(Error::Unsupported); + } + let properties = super::object::write(&wire.properties, self.version)?; + let object = if self.version == Version::Draft14 && wire.status.is_some() { + Some(wire.object_id.unwrap_or(0)) + } else { + wire.object_id }; + let priority = wire.priority.or_else(|| { + (self.version == Version::Draft14).then(|| super::priority::to_wire(self.track.info().priority)) + }); + ( + object, + priority, + wire.end_of_group, + wire.status, + (!properties.is_empty()).then_some(properties), + ) + } + None => { let mut properties = Vec::new(); - let mut w = Encoder::new(&mut properties, self.version.into()); - if ietf::encode_object_time(&mut w, timestamp, timescale, self.version).is_err() { - continue; + if let Some(timescale) = self.timescale { + let timestamp = datagram.timestamp.ok_or(Error::TimestampMismatch)?; + ietf::encode_object_time( + &mut Encoder::new(&mut properties, self.version.into()), + timestamp, + timescale, + self.version, + )?; } - Some(properties) + ( + None, + Some(super::priority::to_wire(self.track.info().priority)), + true, + None, + (!properties.is_empty()).then_some(properties), + ) } - None => None, }; let body = ietf::ObjectDatagram { track_alias: self.request_id.0, group_id: sequence, - object_id: None, - publisher_priority: Some(super::priority::to_wire(self.track.info().priority)), - end_of_group: true, + object_id, + publisher_priority: priority, + end_of_group, properties, - body: ietf::DatagramBody::Payload(datagram.payload), - }; - let Ok(body) = body.encode_bytes(self.version) else { - continue; - }; - + body: match status { + Some(status) => ietf::DatagramBody::Status(status), + None => ietf::DatagramBody::Payload(datagram.datagram.payload), + }, + } + .encode_bytes(self.version)?; let max = self.session.max_datagram_size(); if body.len() > max { tracing::debug!( @@ -3224,6 +3356,7 @@ impl TrackServe { } let _ = self.session.send_datagram(&body); } + Ok(()) } } @@ -3388,7 +3521,9 @@ struct GroupServe { group: group::Consumer, timescale: Option, version: Version, - object_delta: u64, + prior_object: Option, + slice: GroupSlice, + ended: bool, terminal: Option>, marked: bool, state: GroupState, @@ -3408,7 +3543,7 @@ enum GroupState { writer: Writer, frame: Option, chunk: Option, - batch: Box, + batch: Box>, batch_pos: usize, }, /// Every frame is written and the FIN sent: wait for the acknowledgement so a @@ -3440,9 +3575,10 @@ impl GroupServe { version: Version, slice: GroupSlice, ) -> Self { - group.skip_to(slice.skip); - group.end_at(slice.until.map_or(Bound::Unbounded, Bound::Excluded)); - let object_delta = group.index(); + if group.wire().is_none() { + group.skip_to(slice.skip); + group.end_at(slice.until.map_or(Bound::Unbounded, Bound::Excluded)); + } Self { session, opened: Default::default(), @@ -3451,7 +3587,9 @@ impl GroupServe { group, timescale, version, - object_delta, + prior_object: None, + slice, + ended: false, terminal: None, marked: false, state: GroupState::Open, @@ -3506,7 +3644,7 @@ impl GroupServe { writer, frame: None, chunk: None, - batch: Box::new(frame::Buffer::new()), + batch: Box::new(arrayvec::ArrayVec::new()), batch_pos: 0, }; } @@ -3563,32 +3701,54 @@ impl GroupServe { } } else if let Some(pending) = frame { match pending.poll_read_chunk(waiter) { - Poll::Ready(Ok(Some(next))) => *chunk = Some(next), + Poll::Ready(Ok(Some(next))) => { + let id = pending + .fields + .as_ref() + .map_or(self.group.index() - 1, |fields| fields.id); + if id >= self.slice.skip { + *chunk = Some(next); + } + } Poll::Ready(Ok(None)) => *frame = None, Poll::Ready(Err(err)) => break 'serve Err(err), Poll::Pending => return Poll::Pending, } } else if *batch_pos < batch.len() { - let batched = &mut batch.filled_mut()[*batch_pos]; - if let Err(err) = buffer_object_info( - writer, - std::mem::take(&mut self.object_delta), - self.msg.flags.has_extensions, - batched.timestamp, - batched.payload.len() as u64, - self.timescale, - self.version, - ) { - break 'serve Err(err); + let ordinal = self.group.index() - batch.len() as u64 + *batch_pos as u64; + let batched = &mut batch[*batch_pos]; + let id = batched.fields.as_ref().map_or(ordinal, |fields| fields.id); + *batch_pos += 1; + if self.slice.until.is_some_and(|until| id >= until) { + break 'serve Ok(()); } - let payload = std::mem::take(&mut batched.payload); + if id < self.slice.skip { + continue; + } + let delta = object_delta(self.prior_object, id)?; + buffer_object( + writer, + Object { + delta, + timestamp: batched.timestamp, + size: batched.payload.len() as u64, + status: 0, + fields: batched.fields.as_deref(), + }, + ObjectEncoding { + extensions: self.msg.flags.has_extensions, + timescale: self.timescale, + version: self.version, + }, + )?; + self.prior_object = Some(id); + let payload = std::mem::take(&mut batched.frame.payload); if !payload.is_empty() { *chunk = Some(payload); } - *batch_pos += 1; self.group.keep_alive(); } else { - match self.group.poll_read_frames(waiter, batch) { + match self.group.poll_read_objects(waiter, batch) { Poll::Ready(Ok(count)) if count > 0 => { *batch_pos = 0; continue; @@ -3602,33 +3762,72 @@ impl GroupServe { match self.group.poll_next_frame(waiter) { Poll::Ready(Ok(Some(next))) => { - if let Err(err) = buffer_object( - writer, - std::mem::take(&mut self.object_delta), - self.msg.flags.has_extensions, - &next, - self.timescale, - self.version, - ) { - break 'serve Err(err); + let id = + next.fields.as_ref().map_or(self.group.index() - 1, |fields| fields.id); + if self.slice.until.is_some_and(|until| id >= until) { + break 'serve Ok(()); } + if id < self.slice.skip { + // The skipped partial frame still drains before the next object. + *frame = Some(next); + continue; + } + buffer_object( + writer, + Object { + delta: object_delta(self.prior_object, id)?, + timestamp: next.timestamp, + size: next.size, + status: 0, + fields: next.fields.as_deref(), + }, + ObjectEncoding { + extensions: self.msg.flags.has_extensions, + timescale: self.timescale, + version: self.version, + }, + )?; + self.prior_object = Some(id); // An empty object has no payload to stream. if next.size > 0 { *frame = Some(next); } } Poll::Ready(Ok(None)) => { + if !self.ended + && let Some(end) = self.group.end_object() + && end.id >= self.slice.skip && self + .slice + .until + .is_none_or(|until| end.id < until) + { + buffer_object( + writer, + Object { + delta: object_delta(self.prior_object, end.id)?, + timestamp: None, + size: 0, + status: 3, + fields: Some(&end), + }, + ObjectEncoding { + extensions: self.msg.flags.has_extensions, + timescale: None, + version: self.version, + }, + )?; + self.ended = true; + self.prior_object = Some(end.id); + continue; + } // The track ends right after this group's last object, so // the marker rides this stream rather than a separate one. if !self.marked - && self - .terminal - .as_ref() - .and_then(|terminal| terminal.end.get()) - .is_some_and(|end| { - end.group == self.msg.group_id && end.frame == self.group.index() - }) { - writer.buffer_varint(std::mem::take(&mut self.object_delta))?; + && let Some(end) = + self.terminal.as_ref().and_then(|terminal| terminal.end.get()) + && end.group == self.msg.group_id && end.frame == self.group.index() + { + writer.buffer_varint(object_delta(self.prior_object, end.frame)?)?; if self.msg.flags.has_extensions { writer.buffer_varint(0)?; } @@ -3700,57 +3899,60 @@ impl GroupServe { /// Object status: no object at or past this location exists. const END_OF_TRACK: u64 = 0x4; -/// Buffer one object's header and prefix: the id delta, optional extension -/// headers carrying the timestamp, the size, and (for an empty object) the status. -fn buffer_object( - writer: &mut Writer, +/// An object's header, with private relay fields when it came from an IETF peer. +struct Object<'a> { delta: u64, - has_extensions: bool, - frame: &frame::Consumer, + timestamp: Option, + size: u64, + status: u64, + fields: Option<&'a crate::model::object::Fields>, +} + +struct ObjectEncoding { + extensions: bool, timescale: Option, version: Version, -) -> Result<(), Error> { - buffer_object_info( - writer, - delta, - has_extensions, - frame.timestamp, - frame.size, - timescale, - version, - ) } -fn buffer_object_info( +fn object_delta(prior: Option, id: u64) -> Result { + match prior { + None => Ok(id), + Some(prior) => id + .checked_sub(prior) + .and_then(|step| step.checked_sub(1)) + .ok_or(Error::MalformedTrack), + } +} + +fn buffer_object( writer: &mut Writer, - delta: u64, - has_extensions: bool, - timestamp: Option, - size: u64, - timescale: Option, - version: Version, + object: Object<'_>, + encoding: ObjectEncoding, ) -> Result<(), Error> { - writer.buffer_varint(delta)?; - - if let Some(timescale) = timescale.filter(|_| has_extensions) { - // Per-object extension headers carry the frame's presentation timestamp, which - // every frame on a timed track has (see `track::Info::timescale`). - let timestamp = timestamp.ok_or(Error::TimestampMismatch)?; - let mut ext = Vec::new(); - ietf::encode_object_time( - &mut Encoder::new(&mut ext, version.into()), - timestamp, - timescale, - version, - )?; + writer.buffer_varint(object.delta)?; + if encoding.extensions { + let ext = match object.fields { + Some(fields) => super::object::write(&fields.properties, encoding.version)?, + None => { + let mut ext = Vec::new(); + if let Some(timescale) = encoding.timescale { + let timestamp = object.timestamp.ok_or(Error::TimestampMismatch)?; + ietf::encode_object_time( + &mut Encoder::new(&mut ext, encoding.version.into()), + timestamp, + timescale, + encoding.version, + )?; + } + ext + } + }; writer.buffer_varint(ext.len() as u64)?; writer.buffer_raw(&ext); } - - writer.buffer_varint(size)?; - if size == 0 { - // Have to write the object status too: Normal (0). - writer.buffer_varint(0)?; + writer.buffer_varint(object.size)?; + if object.size == 0 { + writer.buffer_varint(object.status)?; } Ok(()) } @@ -10101,8 +10303,7 @@ fn live_edge(track: &track::Consumer) -> LiveEdge { match track.peek_latest() { Some(group) if group.sequence == latest => { - let count = group.frame_count() as u64; - let largest = match count.checked_sub(1) { + let largest = match group.last_object() { Some(object) => Some(Location { group: latest, object }), // A group with no frames yet has no objects, so the largest sits in an // earlier group. Walk back through the cache to find it, or a peer that @@ -10151,7 +10352,7 @@ fn largest_before(track: &track::Consumer, sequence: u64) -> Option { let mut sequence = sequence; loop { let group = track.peek_before(sequence)?; - if let Some(object) = (group.frame_count() as u64).checked_sub(1) { + if let Some(object) = group.last_object() { return Some(Location { group: group.sequence, object, diff --git a/rs/moq-net/src/ietf/subscriber.rs b/rs/moq-net/src/ietf/subscriber.rs index 41ec43fd310..49d38217c35 100644 --- a/rs/moq-net/src/ietf/subscriber.rs +++ b/rs/moq-net/src/ietf/subscriber.rs @@ -3046,13 +3046,13 @@ where ) }; - // Whether a FIRST_OBJECT-clear stream is a group with no head is decided in - // [`Self::open_group`], after it peeks the first Object ID. The bit is only the - // publisher's claim; [`next_object_id`] holds the sequence after that. + // Peek before creating a group: a standalone END_OF_TRACK has no payload + // group, and a fill or resume may already own the prefix. // // The peek inside blocks until the publisher produces the group's first object, so // race it against the subscription going away the same way the group read below is. // Otherwise dropping the local subscriber cannot end this handler. + let passthrough = !fill.read().outstanding() && resume.is_none(); let opened = { let mut opening = track.clone(); let mut open = std::pin::pin!(self.open_group(stream, &mut opening, &fill, resume, &group, &mut reading)); @@ -3089,12 +3089,25 @@ where } }; + if passthrough || producer.wire().is_none() { + let first_id = start.max(producer.frame_count() as u64); + producer.set_wire(group::Wire { + sparse: false, + first_object: group.flags.first_object && first_id == start, + first_id, + extensions: group.flags.has_extensions, + has_end: group.flags.has_end, + fetchable: true, + last: None, + })?; + } + // Guarded: this handler can be dropped at any await below, and a group producer // that dies without a terminal leaves its consumer waiting on nothing. let producer = crate::recv::Group::new(producer); let res = { - let mut ingest = GroupIngest::new(self, &group, timescale, start); + let mut ingest = GroupIngest::new(self, &group, timescale, resume.map(|_| start)); let mut writing = producer.clone(); kio::wait(|waiter| { if let Poll::Ready(err) = track.poll_closed(waiter) { @@ -3157,28 +3170,23 @@ where Ok(()) } - /// Deliver one OBJECT_DATAGRAM as a datagram on its subscription's track: a - /// single-frame group at the Group ID. + /// Deliver an OBJECT_DATAGRAM with private relay metadata on its subscription. /// - /// A malformed datagram is the peer breaking the protocol, so it errors. One the model - /// cannot carry is dropped like any lost datagram: an Object past ID 0 (the group would - /// need a second object), a status other than Normal, or an alias that is not bound - /// yet (the draft lets us drop rather than buffer). + /// A malformed datagram is a protocol error. An unbound alias is dropped, as the + /// draft permits instead of buffering; public readers receive only Normal payloads. pub fn recv_datagram(&self, payload: bytes::Bytes) -> Result<(), Error> { let (datagram, _) = ietf::ObjectDatagram::decode_slice(&payload, self.version)?; let (alias, sequence) = (datagram.track_alias, datagram.group_id); - if datagram.object_id.unwrap_or(0) != 0 { - tracing::debug!(alias, sequence, "dropping a datagram past object 0"); - return Ok(()); - } - let payload = match datagram.body { - ietf::DatagramBody::Payload(payload) => payload, - ietf::DatagramBody::Status(0) => bytes::Bytes::new(), - ietf::DatagramBody::Status(status) => { - tracing::debug!(alias, sequence, status, "dropping a datagram status"); - return Ok(()); + let (payload, status) = match datagram.body { + ietf::DatagramBody::Payload(payload) => (payload, None), + ietf::DatagramBody::Status(status @ (0 | END_OF_GROUP | END_OF_TRACK)) => { + (bytes::Bytes::new(), Some(status)) } + ietf::DatagramBody::Status(1) if matches!(self.version, Version::Draft14 | Version::Draft15) => { + (bytes::Bytes::new(), Some(1)) + } + ietf::DatagramBody::Status(_) => return Err(Error::Session(SessionError::ProtocolViolation)), }; let mut state = self.state.lock(); @@ -3194,21 +3202,26 @@ where }; // Like a subgroup object: a track that declared no timescale is untimed. One on a - // timed track without a Timestamp is refused by the model and dropped below. - let timestamp = match (track.timescale, &datagram.properties) { - (Some(timescale), Some(properties)) => { - let mut properties = Decoder::new(properties, self.version.into()); - ietf::decode_object_time(&mut properties, timescale, self.version)? - } - _ => None, - }; - + // timed track without a Timestamp is refused by the model. + let properties = bytes::Bytes::from(datagram.properties.unwrap_or_default()); + let decoded = super::object::read(&properties, self.version, track.timescale)?; let Some(producer) = track.producer.as_mut() else { return Ok(()); }; - if let Err(err) = producer.insert_datagram(sequence, timestamp, payload) { - tracing::debug!(%err, alias, sequence, "dropping datagram"); - } + producer.insert_wire_datagram(crate::model::datagram::Stored { + datagram: crate::Datagram { + sequence, + timestamp: decoded.timestamp, + payload, + }, + wire: Some(std::sync::Arc::new(crate::model::datagram::Wire { + object_id: datagram.object_id, + priority: datagram.publisher_priority, + end_of_group: datagram.end_of_group, + status, + properties: decoded.properties, + })), + })?; Ok(()) } } @@ -3328,34 +3341,8 @@ where return Ok(Opened::EndOfTrack(first.id)); } - // FIRST_OBJECT clear is the publisher's claim that the stream starts partway - // through the group. The first Object ID is absolute either way, and IDs start - // at 0, so a clear bit on object 0 is still the head: the publisher is out of - // spec, not missing a keyframe. Any other first ID, or a stream with no object, - // has a hole at the front. Drop it before a group exists and resume at the next - // group, the same as a publisher that no longer holds the head. - // - // Drafts 14-17 have no FIRST_OBJECT bit, so only a non-zero first ID marks the - // hole there; a stream with no object is an empty group. - // - // A fill we asked for is the exception: its fetch stream is carrying the head - // this tail stitches onto. So is the group a resumed subscription asked to start - // partway through, whose head came from another route. - let headless = match self.version { - Version::Draft14 | Version::Draft15 | Version::Draft16 | Version::Draft17 => { - first.is_some_and(|first| first.id != 0) - } - _ => !header.flags.first_object && first.is_none_or(|first| first.id != 0), - }; - if headless && !fill.read().outstanding() && resume.is_none() { - tracing::debug!( - track_alias = %header.track_alias, - group = %header.group_id, - object = first.map(|first| first.id), - "dropping a group with no head" - ); - return Err(Error::Unsupported); - } + // Without a fill or resume, a relay preserves the subgroup's first Object ID + // and FIRST_OBJECT claim independently. Plain readers see only frame ordinals. if !fill.read().outstanding() { // The group a resumed subscription picks up partway through starts where it @@ -3369,7 +3356,7 @@ where producer.start_at(resume.frame)?; return Ok(Opened::Group(producer, first.map_or(resume.frame, |first| first.id))); } - return Ok(Opened::Group(create(track)?, 0)); + return Ok(Opened::Group(create(track)?, first.map_or(0, |first| first.id))); } // The first object's ID delta is its absolute Object ID (see `next_object_id`). @@ -3466,7 +3453,9 @@ struct GroupIngest { timescale: Option, version: Version, prior_object: Option, - start: u64, + start: Option, + properties: crate::model::object::Properties, + gap: Option, /// The object being read is one the group started past, so it is read and dropped. dropping: bool, phase: IngestPhase, @@ -3497,7 +3486,7 @@ impl GroupIngest { subscriber: &Subscriber, group: &ietf::GroupHeader, timescale: Option, - start: u64, + start: Option, ) -> Self { Self { has_extensions: group.flags.has_extensions, @@ -3505,6 +3494,8 @@ impl GroupIngest { version: subscriber.version, prior_object: None, start, + properties: Default::default(), + gap: None, dropping: false, phase: IngestPhase::Delta, budget: subscriber.frames.clone(), @@ -3689,11 +3680,10 @@ where /// Decode fetch objects into `head`, creating each group from the first object's /// absolute IDs. /// - /// A relative join and a draft-20 fill are one group: objects numbered from the start - /// with no gaps. An absolute join spans whole groups up to the subscribe's Largest - /// Location; each complete group below that is finished, and the last one is the head - /// the live stream continues. Anything else is a head the model cannot represent, and - /// refusing the stream leaves the subscription itself alone. + /// A relative join and a draft-20 fill are one group. An absolute join spans whole + /// groups up to Largest Location; the last is the head the live stream continues. + /// A Prior Object ID Gap must account for every missing ID. Ordinal resume still + /// requires contiguous IDs; refusing its fill leaves the subscription alone. /// /// A pre-draft-20 join resuming partway through a group asks for all of it, so the /// objects below `resume` are dropped: another route already delivered them. @@ -3744,54 +3734,53 @@ where continue; } - match head.as_ref().map(|(sequence, next, _)| (*sequence, *next)) { - None => { - let (Some(sequence), Some(0)) = (group, object.object) else { - tracing::warn!( - group = ?group, - object = ?object.object, - "a fill must start at a group's first object" - ); - return Err(Error::Unsupported); - }; - open_fill_group(track, head, sequence)?; - } - Some((sequence, _)) if group.is_some_and(|group| group != sequence) => { - let Some(group) = group else { - unreachable!("the filter above proved group is Some"); - }; - if object.object != Some(0) { - tracing::warn!( - group, - object = ?object.object, - "a fill must start at a group's first object" - ); - return Err(Error::Unsupported); - } - advance_fill_group(track, head, group, joining, largest)?; + let decoded = super::object::read( + &bytes::Bytes::from(object.properties.unwrap_or_default()), + self.version, + timescale, + )?; + let expected = head + .as_ref() + .filter(|(sequence, _, _)| group.is_none_or(|group| group == *sequence)) + .map_or(0, |(_, next, _)| *next); + let id = match (object.group.is_some(), object.object) { + (_, Some(id)) + if matches!( + self.version, + Version::Draft14 | Version::Draft15 | Version::Draft16 | Version::Draft17 + ) => + { + id } - Some((sequence, next)) => { - let id = match (object.group.is_some(), object.object) { - (true, Some(id)) => id, - (false, None | Some(1)) => next, - _ => { - tracing::warn!( - sequence, - next, - object = ?object.object, - "fill object IDs must increment by 1" - ); - return Err(Error::Unsupported); - } - }; - if id != next { - tracing::warn!(sequence, next, object = id, "fill object IDs must increment by 1"); - return Err(Error::Unsupported); - } + (true, Some(id)) => id, + (false, Some(delta)) => expected + .checked_sub(1) + .and_then(|prior| prior.checked_add(delta)) + .ok_or(Error::ProtocolViolation)?, + (false, None) => expected, + _ => return Err(Error::ProtocolViolation), + }; + if id < expected { + return Err(Error::ProtocolViolation); + } + if id > expected && (resume.is_some() || decoded.gap.is_none_or(|gap| gap < id - expected)) { + return Err(Error::Unsupported); + } + match head.as_ref().map(|(sequence, _, _)| *sequence) { + None => open_fill_group(track, head, group.ok_or(Error::ProtocolViolation)?)?, + Some(sequence) if group.is_some_and(|group| group != sequence) => { + advance_fill_group(track, head, group.expect("new group"), joining, largest)?; } + _ => {} } let (sequence, next, producer) = head.as_mut().expect("the head was created above"); + if *next == 0 + && let Some(mut wire) = producer.wire() + { + wire.first_id = id; + producer.set_wire(wire)?; + } if *next == 0 && let Some(resume) = resume.filter(|resume| resume.group == *sequence) { @@ -3799,12 +3788,21 @@ where } let keep = *next >= producer.frame_count() as u64; if !self - .recv_fetch_payload(stream, producer, object.properties, timescale, keep) + .recv_fetch_payload( + stream, + producer, + FetchPayload { + decoded, + timescale, + keep, + id, + }, + ) .await? { return Err(Error::Unsupported); } - *next += 1; + *next = id.checked_add(1).ok_or(Error::ProtocolViolation)?; } Ok(()) @@ -3819,28 +3817,31 @@ where &self, stream: &mut Reader, producer: &mut group::Producer, - properties: Option>, - timescale: Option, - keep: bool, + payload: FetchPayload, ) -> Result { - // The properties carry the frame's presentation timestamp (the Timestamp Object - // Property) in the units the track declared. A track that declared none is - // untimed, and an object on a timed track without one is malformed. - let timestamp = match (properties, timescale) { - (Some(properties), Some(timescale)) => { - let mut properties = Decoder::new(&properties, self.version.into()); - ietf::decode_object_time(&mut properties, timescale, self.version)? - } - _ => None, - }; - - // A fetch object has no status field from draft-16 on; a zero length is simply - // an empty object. Draft-14 and 15 still encode Normal (0) after a zero length. + let FetchPayload { + decoded, + timescale, + keep, + id, + } = payload; + if keep { + producer.mark_gap(id, decoded.gap)?; + } let size = stream.varint().await?; if size == 0 && matches!(self.version, Version::Draft14 | Version::Draft15) { match stream.varint().await? { 0 => {} - END_OF_GROUP | END_OF_TRACK => return Ok(false), + END_OF_GROUP => { + if keep { + producer.set_end_object(crate::model::object::Fields { + id, + properties: decoded.properties, + })?; + } + return Ok(false); + } + END_OF_TRACK => return Ok(false), _ => return Err(Error::Unsupported), } } @@ -3849,11 +3850,21 @@ where std::future::poll_fn(|cx| stream.poll_skip(cx, &mut remaining)).await?; return Ok(true); } - - let timestamp = object_time(timescale, timestamp)?; - // `create_frame_owned` is the allocation chokepoint: it rejects an oversized `size` - // and allocates up front only within the budget, so no pre-check is needed. - let mut frame = producer.create_frame_owned(frame::Info { size, timestamp }, &self.frames)?; + let timestamp = object_time(timescale, decoded.timestamp)?; + let plain = id == producer.frame_count() as u64 + && decoded.properties.is_plain() + && decoded.properties.timestamp == timestamp.map(|time| time.value()); + let fields = (!plain).then(|| { + std::sync::Arc::new(crate::model::object::Fields { + id, + properties: decoded.properties, + }) + }); + let header = frame::Header { + info: frame::Info { size, timestamp }, + fields, + }; + let mut frame = producer.create_frame_owned(header, &self.frames)?; if let Err(err) = std::future::poll_fn(|cx| stream.poll_read_frame(cx, &mut frame)).await { let _ = frame.abort(err.clone()); return Err(err); @@ -4190,6 +4201,15 @@ where }) .await; let (producer, timescale, start, end) = taken?; + producer.set_wire(group::Wire { + sparse: false, + first_object: start == 0, + first_id: start, + extensions: true, + has_end: true, + fetchable: true, + last: None, + })?; let closed = producer.clone(); let mut producer = crate::recv::Group::new(producer); @@ -4221,8 +4241,8 @@ where res } - /// Decode one group's objects: all in the producer's group, numbered from `start` with - /// no gaps, and never past `end` (exclusive) when FETCH_OK named one inside the group. + /// Decode one group's objects, preserving marked gaps and their properties, never + /// past `end` (exclusive) when FETCH_OK named one inside the group. /// Before draft-20 they must also reach it. async fn recv_group_fetch_objects( &self, @@ -4256,26 +4276,47 @@ where prior_group = Some(group); } let id = match (object.group.is_some(), object.object) { - (true, Some(id)) => Some(id), - (false, None | Some(1)) => Some(next), - _ => None, + (_, Some(id)) + if matches!( + self.version, + Version::Draft14 | Version::Draft15 | Version::Draft16 | Version::Draft17 + ) => + { + id + } + (true, Some(id)) => id, + (false, Some(delta)) => next + .checked_sub(1) + .and_then(|prior| prior.checked_add(delta)) + .ok_or(Error::ProtocolViolation)?, + (false, None) => next, + _ => return Err(Error::ProtocolViolation), }; - // Another group, or an ID before the next one, is outside what was asked for. - if group.is_some_and(|group| group != sequence) || id.is_some_and(|id| id < next) { - tracing::warn!(sequence, next, group = ?group, object = ?id, "a group fetch answered outside its range"); + if group.is_some_and(|group| group != sequence) || id < next { return Err(Error::ProtocolViolation); } - // A skipped ID is an object that does not exist, a hole the model cannot hold. - if id != Some(next) { - tracing::warn!(sequence, next, object = ?id, "a group fetch skipped an object"); + let decoded = super::object::read( + &bytes::Bytes::from(object.properties.unwrap_or_default()), + self.version, + timescale, + )?; + if id > next && decoded.gap.is_none_or(|gap| gap < id - next) { return Err(Error::Unsupported); } - match self - .recv_fetch_payload(stream, producer, object.properties, timescale, true) + .recv_fetch_payload( + stream, + producer, + FetchPayload { + decoded, + timescale, + keep: true, + id, + }, + ) .await? { - true => next += 1, + true => next = id.checked_add(1).ok_or(Error::ProtocolViolation)?, false => ended = true, } if end.is_some_and(|end| next > end) { @@ -4301,6 +4342,13 @@ where } } +struct FetchPayload { + decoded: super::object::Decoded, + timescale: Option, + keep: bool, + id: u64, +} + /// One Object header on a fetch stream, after version-specific decoding. struct FetchedObject { group: Option, @@ -4401,6 +4449,15 @@ fn open_fill_group( sequence: u64, ) -> Result<(), Error> { let producer = track.create_group(group::Info { sequence })?; + producer.set_wire(group::Wire { + sparse: false, + first_object: true, + first_id: 0, + extensions: true, + has_end: true, + fetchable: true, + last: None, + })?; *head = Some((sequence, 0, crate::recv::Group::new(producer))); Ok(()) } @@ -4497,6 +4554,21 @@ fn object_time( } impl GroupIngest { + fn header(&mut self, group: &group::Producer, info: frame::Info) -> Result { + let id = self.prior_object.expect("payload follows object ID"); + group.mark_gap(self.prior_object.expect("properties follow Object ID"), self.gap)?; + let plain = id == group.frame_count() as u64 + && self.properties.is_plain() + && self.properties.timestamp == info.timestamp.map(|time| time.value()); + let fields = (!plain).then(|| { + std::sync::Arc::new(crate::model::object::Fields { + id, + properties: std::mem::take(&mut self.properties), + }) + }); + Ok(frame::Header { info, fields }) + } + /// `Ready(Ok(_))` once the stream FINs on an object boundary, or an explicit /// end-of-group or end-of-track status arrives. The caller finishes or aborts the /// group; an object cut short mid-payload was already aborted here with the reason. @@ -4515,7 +4587,9 @@ impl GroupIngest { }; let id = next_object_id(self.prior_object, id_delta, self.start)?; self.prior_object = Some(id); - self.dropping = id < group.frame_count() as u64; + self.properties = Default::default(); + self.gap = None; + self.dropping = self.start.is_some() && id < group.frame_count() as u64; self.phase = match self.has_extensions { true => IngestPhase::ExtSize, false => IngestPhase::Size { timestamp: None }, @@ -4526,18 +4600,11 @@ impl GroupIngest { self.phase = IngestPhase::ExtBytes { size }; } IngestPhase::ExtBytes { size } => { - // Per-object extension headers may carry the frame's presentation - // timestamp (the Timestamp Object Property), in the units the track - // declared. A track that declared no timescale is untimed, even if an - // object carries a Timestamp: it has no units to read it in. let ext = ready!(reader.poll_read_exact(&mut cx, *size))?; - let timestamp = match self.timescale { - Some(timescale) => { - let mut ext = Decoder::new(&ext, self.version.into()); - ietf::decode_object_time(&mut ext, timescale, self.version)? - } - None => None, - }; + let decoded = super::object::read(&ext, self.version, self.timescale)?; + let timestamp = decoded.timestamp; + self.properties = decoded.properties; + self.gap = decoded.gap; self.phase = IngestPhase::Size { timestamp }; } IngestPhase::Size { timestamp } => { @@ -4555,7 +4622,8 @@ impl GroupIngest { // oversized `size` and allocates up front only within the budget, so // no pre-check is needed. let timestamp = object_time(self.timescale, *timestamp)?; - let frame = group.create_frame_owned(frame::Info { size, timestamp }, &self.budget)?; + let header = self.header(group, frame::Info { size, timestamp })?; + let frame = group.create_frame_owned(header, &self.budget)?; self.phase = IngestPhase::Payload { frame }; } IngestPhase::Status { timestamp } => { @@ -4563,11 +4631,17 @@ impl GroupIngest { if status == 0 { if !self.dropping { let timestamp = object_time(self.timescale, *timestamp)?; - let frame = group.create_frame_owned(frame::Info { size: 0, timestamp }, &self.budget)?; + let header = self.header(group, frame::Info { size: 0, timestamp })?; + let frame = group.create_frame_owned(header, &self.budget)?; frame.finish()?; } self.phase = IngestPhase::Delta; } else if status == END_OF_GROUP { + group.mark_gap(self.prior_object.expect("properties follow Object ID"), self.gap)?; + group.set_end_object(crate::model::object::Fields { + id: self.prior_object.expect("status follows object ID"), + properties: std::mem::take(&mut self.properties), + })?; // Allowed even when the header marks the group's end: that bit only // lets a FIN imply it, and imquic sends both. self.phase = IngestPhase::Finished(Ended::Group); @@ -5233,7 +5307,7 @@ mod tests { }; let payload = |bytes: &'static [u8]| ietf::DatagramBody::Payload(bytes::Bytes::from_static(bytes)); - // Dropped: a second object in the group, an unbound alias, and a status. + // Nonzero objects are delivered; an unbound alias and non-payload statuses stay invisible to plain readers. subscriber .recv_datagram(datagram(7, 4, Some(1), payload(b"no"))) .unwrap(); @@ -5245,13 +5319,16 @@ mod tests { subscriber .recv_datagram(datagram(7, 9, Some(0), payload(b"yes"))) .unwrap(); + let first = consumer.recv_datagram().now_or_never().unwrap().unwrap().unwrap(); + assert_eq!(first.sequence, 4); + assert_eq!(&first.payload[..], b"no"); let received = consumer.recv_datagram().now_or_never().unwrap().unwrap().unwrap(); assert_eq!(received.sequence, 9, "the Group ID is the sequence"); assert_eq!(received.timestamp, Some(timestamp)); assert_eq!(&received.payload[..], b"yes"); assert!( consumer.recv_datagram().now_or_never().is_none(), - "only one got through" + "only the payload datagrams got through" ); // A status datagram cannot end the group. @@ -7807,17 +7884,11 @@ fn subscribe_join( }) } -/// The absolute Object ID for a subgroup object, given the prior one and its delta. +/// Resolve the first absolute ID or a later ID delta without overflowing. /// -/// The first object's delta is its absolute Object ID; every later one is the prior ID plus -/// the delta plus one. moq-lite groups never skip an object, so a gap is refused: it would -/// renumber every frame after it. Checked against the ID rather than the header's -/// FIRST_OBJECT bit, which is only the publisher's claim. -/// -/// `start` is where this stream picks the group up, which is 0 for a group delivered whole -/// and the object after the fill's head for the tail of a stitched one. Anything else has a -/// hole at the front. -fn next_object_id(prior: Option, delta: u64, start: u64) -> Result { +/// Ordinary subgroup relay ingest preserves gaps. A fill or resume supplies `start` +/// and requires contiguous IDs because its public frame ordinals stitch onto another copy. +fn next_object_id(prior: Option, delta: u64, start: Option) -> Result { let object = match prior { None => delta, Some(prior) => prior @@ -7826,8 +7897,8 @@ fn next_object_id(prior: Option, delta: u64, start: u64) -> Result, + ingress: Version, + egress: Version, + fetch: Option<(u64, u64)>, + ) -> (Vec, crate::lite::test_transport::Log) { + use crate::lite::test_transport::Sent; + let origin = crate::origin::Config::new(crate::Hop::new(2).unwrap()).produce(); + let broadcast = origin.publish("room", crate::origin::Route::default()).unwrap(); + let track = broadcast.create_track("video", None).unwrap(); + let mut h = Harness::with_timescale(Fill::Done, vec![script], None); + h.subscriber.version = ingress; + h.subscriber.state.lock().subscribes.get_mut(&REQUEST).unwrap().producer = Some(track.clone()); + let mut stream = h.stream().await.with_version(ingress); + h.subscriber.clone().recv_group(&mut stream).await.unwrap(); + track.finish().unwrap(); + + let mut downstream = ScriptedSession::per_stream(vec![vec![]]); + let peer = peer::PeerSetup::default(); + peer.set(peer::Peer::default()); + let publisher = crate::ietf::Publisher::new( + crate::time::Clock::sim(), + downstream.clone(), + origin.consume(), + Control::new(None, false), + None, + peer, + egress, + ); + let mut body = Vec::new(); + let id = if let Some((start, end)) = fetch { + let fetch_type = if Filter::is_draft20(egress) { + ietf::FetchType::Filtered { + namespace: Path::new("room"), + track: "video".into(), + filter: Filter::Absolute { + start: ietf::Location { + group: SEQUENCE, + object: start, + }, + end: Some(ietf::EndLocation { + group: SEQUENCE, + object: Some(end - 1), + }), + }, + } + } else { + ietf::FetchType::Standalone { + namespace: Path::new("room"), + track: "video".into(), + start: ietf::Location { + group: SEQUENCE, + object: start, + }, + end: ietf::Location { + group: SEQUENCE, + object: end, + }, + } + }; + ietf::Fetch { + request_id: RequestId(3), + subscriber_priority: 128, + group_order: ietf::GroupOrder::Ascending, + fetch_type, + range_filters: false, + fill_timeout: false, + properties_wanted: true, + } + .encode_msg(&mut Encoder::new(&mut body, egress.into()), egress) + .unwrap(); + ietf::Fetch::ID + } else { + ietf::Subscribe { + request_id: RequestId(3), + track_namespace: Path::new("room"), + track_name: "video".into(), + subscriber_priority: 128, + group_order: ietf::GroupOrder::Ascending, + filter: Filter::Unfiltered, + fill: None, + properties_wanted: true, + forward: true, + range_filters: false, + } + .encode_msg(&mut Encoder::new(&mut body, egress.into()), egress) + .unwrap(); + ietf::Subscribe::ID + }; + let stream = Stream::open(&mut downstream, egress).await.unwrap(); + publisher + .handle_stream(id, ietf::Body(body.into()), stream) + .unwrap() + .await; + let mut streams = std::collections::BTreeMap::>::new(); + for (id, event) in downstream.log.trail() { + if let Sent::Write(bytes) = event { + streams.entry(id).or_default().extend(bytes); + } + } + let bytes = if fetch.is_some() { + streams.into_values().flatten().collect() + } else { + streams + .into_values() + .find(|bytes| { + ietf::GroupHeader::decode(&mut Decoder::new(bytes, egress.into()), egress) + .is_ok_and(|header| header.group_id == SEQUENCE) + }) + .expect("the relay emitted the group") + }; + (bytes, downstream.log) + } + + async fn relay_passthrough_stream( + script: Vec, + ingress: Version, + egress: Version, + ) -> (Vec, crate::lite::test_transport::Log) { + relay_passthrough(script, ingress, egress, None).await + } + #[moq_net_sim::test] - async fn a_gapped_object_leaves_the_track_flowing() { - let mut gapped = tail_stream(SEQUENCE, 0, &[b"head"]); - crate::coding::Encoder::new(&mut gapped, VERSION.into()) - .varint(1u64) + async fn passthrough_preserves_object_properties_through_dispatch() { + for version in [Version::Draft15, Version::Draft22] { + let mut script = Vec::new(); + ietf::GroupHeader { + track_alias: ALIAS, + group_id: SEQUENCE, + sub_group_id: 0, + publisher_priority: 0, + flags: ietf::GroupFlags { + has_extensions: true, + ..Default::default() + }, + } + .encode(&mut Encoder::new(&mut script, version.into()), version) .unwrap(); - let h = Harness::new(Fill::Done, vec![gapped, tail_stream(SEQUENCE + 1, 0, &[b"ok"])]); - let mut consumer = h.track.subscribe(None); - let mut stream = h.stream().await; - let result = h.subscriber.clone().recv_group(&mut stream).await; - assert!(matches!(result, Err(Error::Unsupported)), "{result:?}"); - let mut stream = h.stream().await; - h.subscriber.clone().recv_group(&mut stream).await.unwrap(); - assert_eq!(read_group(&mut consumer).await.1[0].1, b"ok"); - assert!(h.session.log.closes().is_empty()); + let properties = match version { + Version::Draft15 => vec![2, 7, 5, 1, 42], + _ => vec![2, 7, 3, 1, 42], + }; + let mut encoder = Encoder::new(&mut script, version.into()); + encoder.varint(0u64).unwrap(); + encoder.bytes(&properties).unwrap(); + encoder.varint(1u64).unwrap(); + script.push(99); + let (bytes, _) = relay_passthrough_stream(script, version, version).await; + let mut decoder = Decoder::new(&bytes, version.into()); + let header = ietf::GroupHeader::decode(&mut decoder, version).unwrap(); + assert!(header.flags.has_extensions, "{version}: properties were dropped"); + assert_eq!(decoder.varint().unwrap(), 0); + assert_eq!(decoder.bytes().unwrap(), properties); + assert_eq!(decoder.varint().unwrap(), 1); + assert_eq!(decoder.slice(1).unwrap(), &[99]); + } + } + + fn object_stream(version: Version, first: bool, objects: &[(u64, &[u8], Option)]) -> Vec { + let mut bytes = Vec::new(); + ietf::GroupHeader { + track_alias: ALIAS, + group_id: SEQUENCE, + sub_group_id: 0, + publisher_priority: 128, + flags: ietf::GroupFlags { + first_object: first, + has_extensions: true, + has_end: true, + ..Default::default() + }, + } + .encode(&mut Encoder::new(&mut bytes, version.into()), version) + .unwrap(); + let mut previous = None; + for &(id, properties, status) in objects { + let delta = previous.map_or(id, |previous| id - previous - 1); + previous = Some(id); + let mut encoder = Encoder::new(&mut bytes, version.into()); + encoder.varint(delta).unwrap(); + encoder.bytes(properties).unwrap(); + encoder.varint(u64::from(status.is_none())).unwrap(); + if let Some(status) = status { + encoder.varint(status).unwrap(); + } else { + bytes.push(99); + } + } + bytes + } + + #[moq_net_sim::test] + async fn passthrough_fetch_requires_marked_gaps_and_preserves_fields() { + for version in [Version::Draft15, Version::Draft17, Version::Draft22] { + for marked in [false, true] { + let properties = if marked { vec![0x3e, 6] } else { vec![] }; + let second = if marked { vec![0x3e, 1] } else { vec![] }; + for start in [0, 7] { + let script = object_stream(version, true, &[(6, &properties, None), (8, &second, None)]); + let (bytes, log) = relay_passthrough(script, version, version, Some((start, 9))).await; + let mut decoder = Decoder::new(&bytes, version.into()); + if !marked { + assert_eq!(decoder.varint().unwrap(), ietf::RequestError::ID); + assert_eq!(ietf::RequestError::decode(&mut decoder, version).unwrap().error_code, 3); + assert!(decoder.is_empty()); + continue; + } + assert_eq!(decoder.varint().unwrap(), ietf::FetchOk::ID); + let ok = ietf::FetchOk::decode(&mut decoder, version).unwrap(); + assert_eq!( + ok.end_location.object, + if Filter::is_draft20(version) { 8 } else { 9 }, + "{version} start={start}" + ); + assert_eq!(decoder.varint().unwrap(), ietf::FetchHeader::TYPE); + ietf::FetchHeader::decode(&mut decoder, version).unwrap(); + let mut prior = None; + for (id, properties) in [(6, &properties), (8, &second)] + .into_iter() + .filter(|(id, _)| *id >= start) + { + let ietf::FetchObject::Object { + object, + properties: actual, + .. + } = ietf::FetchObject::decode(&mut decoder, version).unwrap() + else { + panic!("object"); + }; + let wire = if version == Version::Draft22 { + prior.map_or(id, |prior| id - prior) + } else { + id + }; + assert_eq!(object, Some(wire), "{version}"); + assert_eq!(actual.as_ref(), Some(properties)); + assert_eq!(decoder.varint().unwrap(), 1); + assert_eq!(decoder.slice(1).unwrap(), &[99]); + prior = Some(id); + } + assert!(decoder.is_empty()); + assert!(log.resets().is_empty()); + } + } + } + } + + #[moq_net_sim::test] + async fn passthrough_refuses_contradictory_gap_properties() { + for properties in [vec![0x3e, 2], vec![0x3e, 3], vec![0x3e, 1, 0, 1]] { + let script = object_stream(VERSION, true, &[(0, &[], None), (2, &properties, None)]); + let h = Harness::with_timescale(Fill::Done, vec![script], None); + let mut stream = h.stream().await; + assert!(matches!( + h.subscriber.clone().recv_group(&mut stream).await, + Err(Error::MalformedTrack) + )); + } + } + + #[moq_net_sim::test] + async fn passthrough_fetch_ingest_keeps_marked_gaps_and_stitches_its_tail() { + for version in [Version::Draft15, Version::Draft17, Version::Draft22] { + let mut objects = Vec::new(); + for (index, id) in [6u64, 8].into_iter().enumerate() { + let properties = vec![0x3e, if index == 0 { 6 } else { 1 }]; + ietf::FetchObject::Object { + subgroup: ietf::FetchSubgroup::Zero, + group: (index == 0).then_some(SEQUENCE), + object: Some(if version == Version::Draft22 && index > 0 { + 2 + } else { + id + }), + priority: (index == 0).then_some(128), + properties: Some(properties), + } + .encode(&mut Encoder::new(&mut objects, version.into()), version) + .unwrap(); + Encoder::new(&mut objects, version.into()).varint(1u64).unwrap(); + objects.push(99); + } + let mut run = GroupFetchRun::new(version, objects.clone()).await; + let mut producer = run.track.create_group(group::Info { sequence: SEQUENCE }).unwrap(); + run.subscriber + .recv_group_fetch_objects(&mut run.stream, &mut producer, 0, Some(9), None) + .await + .unwrap(); + producer.finish().unwrap(); + let mut reader = producer.consume(); + for id in [6, 8] { + let frame = kio::wait(|waiter| reader.poll_read_object(waiter)) + .await + .unwrap() + .unwrap(); + assert_eq!(frame.fields.as_ref().unwrap().id, id); + assert_eq!(frame.fields.as_ref().unwrap().properties.values[0].id, 0x3e); + } + + let tail = object_stream(version, false, &[(9, &[], None)]); + let mut h = Harness::with_timescale(Fill::Serving(None), vec![tail], None); + h.subscriber.version = version; + let mut run = GroupFetchRun::new(version, objects).await; + let fill = h + .subscriber + .run_fill(&mut run.stream, &mut h.track, None, None, None, None) + .await + .unwrap(); + *h.fill.write().ok().expect("fill open") = fill; + let mut consumer = h.track.subscribe(None); + let mut tail = h.stream().await.with_version(version); + h.subscriber.clone().recv_group(&mut tail).await.unwrap(); + let mut group = consumer.recv_group().await.unwrap().unwrap(); + assert_eq!(group.wire().unwrap().first_id, 6); + assert!( + group.wire().unwrap().fetchable, + "a contiguous tail adds no gap to the FETCH head" + ); + for id in [6, 8, 9] { + let frame = kio::wait(|waiter| group.poll_read_object(waiter)) + .await + .unwrap() + .unwrap(); + assert_eq!(frame.fields.as_ref().unwrap().id, id); + } + assert!(group.read_frame().await.unwrap().is_none()); + } + } + + #[moq_net_sim::test] + async fn passthrough_preserves_ids_first_object_and_end_status() { + for version in [Version::Draft15, Version::Draft22] { + for first in [false, true] { + let objects = [ + (6, &[][..], None), + (8, &[][..], None), + (11, &[][..], Some(END_OF_GROUP)), + ]; + let script = object_stream(version, first, &objects); + let (bytes, log) = relay_passthrough_stream(script, version, version).await; + assert!(log.resets().is_empty()); + let mut decoder = Decoder::new(&bytes, version.into()); + let header = ietf::GroupHeader::decode(&mut decoder, version).unwrap(); + assert_eq!(header.flags.first_object, version == Version::Draft15 || first); + for (delta, status) in [(6, None), (1, None), (2, Some(END_OF_GROUP))] { + assert_eq!(decoder.varint().unwrap(), delta); + assert!(decoder.bytes().unwrap().is_empty()); + assert_eq!(decoder.varint().unwrap(), u64::from(status.is_none())); + if let Some(status) = status { + assert_eq!(decoder.varint().unwrap(), status); + } else { + assert_eq!(decoder.slice(1).unwrap(), &[99]); + } + } + assert!(decoder.is_empty()); + } + } + } + + #[moq_net_sim::test] + async fn passthrough_preserves_immutable_bytes_and_refuses_incompatible_drafts() { + for ingress in [ + Version::Draft15, + Version::Draft16, + Version::Draft17, + Version::Draft18, + Version::Draft22, + ] { + let mut inner = Vec::new(); + let mut encoder = Encoder::new(&mut inner, ingress.into()); + for value in [2u64, 96_000, if ingress == Version::Draft15 { 4 } else { 2 }, 128] { + encoder.varint(value).unwrap(); + } + encoder + .varint(if ingress == Version::Draft15 { 5u64 } else { 1u64 }) + .unwrap(); + encoder.bytes(&[42; 130]).unwrap(); + let mut properties = Vec::new(); + let mut encoder = Encoder::new(&mut properties, ingress.into()); + encoder.varint(0x0bu64).unwrap(); + encoder.bytes(&inner).unwrap(); + for egress in [ + Version::Draft15, + Version::Draft16, + Version::Draft17, + Version::Draft18, + Version::Draft22, + ] { + let script = object_stream(ingress, true, &[(0, &properties, None)]); + let (bytes, log) = relay_passthrough_stream(script, ingress, egress).await; + if super::super::object::encoding(ingress) != super::super::object::encoding(egress) { + assert!(!log.resets().is_empty(), "incompatible immutable bytes must be refused"); + continue; + } + assert!(log.resets().is_empty()); + let mut decoder = Decoder::new(&bytes, egress.into()); + ietf::GroupHeader::decode(&mut decoder, egress).unwrap(); + assert_eq!(decoder.varint().unwrap(), 0); + assert_eq!(decoder.bytes().unwrap(), properties); + } + } + } + + #[moq_net_sim::test] + async fn passthrough_preserves_datagram_objects_through_dispatch() { + for version in [Version::Draft15, Version::Draft22] { + for ranged in [false, true] + .into_iter() + .filter(|ranged| !*ranged || version == Version::Draft22) + { + let origin = crate::origin::Config::new(crate::Hop::new(2).unwrap()).produce(); + let broadcast = origin.publish("room", crate::origin::Route::default()).unwrap(); + let track = broadcast.create_track("video", None).unwrap(); + let mut h = Harness::with_timescale(Fill::Done, vec![], None); + h.subscriber.version = version; + h.subscriber.state.lock().subscribes.get_mut(&REQUEST).unwrap().producer = Some(track.clone()); + let properties = if version == Version::Draft15 { + vec![2, 7, 5, 1, 42] + } else { + vec![2, 7, 3, 1, 42] + }; + let mut original = Vec::new(); + for id in [6, 7, 8] { + let datagram = ietf::ObjectDatagram { + track_alias: ALIAS, + group_id: SEQUENCE, + object_id: Some(id), + publisher_priority: Some(200), + end_of_group: false, + properties: (id != 8).then(|| properties.clone()), + body: if id == 8 { + ietf::DatagramBody::Status(END_OF_GROUP) + } else { + ietf::DatagramBody::Payload(bytes::Bytes::from_static(b"object")) + }, + }; + h.subscriber + .recv_datagram(datagram.encode_bytes(version).unwrap()) + .unwrap(); + original.push(datagram); + } + track.finish().unwrap(); + let mut downstream = ScriptedSession::per_stream(vec![vec![]]).with_datagrams(65535); + let peer = peer::PeerSetup::default(); + peer.set(peer::Peer::default()); + let publisher = crate::ietf::Publisher::new( + crate::time::Clock::sim(), + downstream.clone(), + origin.consume(), + Control::new(None, false), + None, + peer, + version, + ); + let subscribe = ietf::Subscribe { + request_id: RequestId(3), + track_namespace: Path::new("room"), + track_name: "video".into(), + subscriber_priority: 128, + group_order: ietf::GroupOrder::Ascending, + filter: if ranged { + Filter::Absolute { + start: ietf::Location { + group: SEQUENCE, + object: 7, + }, + end: Some(ietf::EndLocation { + group: SEQUENCE, + object: Some(7), + }), + } + } else { + Filter::Unfiltered + }, + fill: None, + properties_wanted: true, + forward: true, + range_filters: false, + }; + let mut body = Vec::new(); + subscribe + .encode_msg(&mut Encoder::new(&mut body, version.into()), version) + .unwrap(); + let stream = Stream::open(&mut downstream, version).await.unwrap(); + publisher + .handle_stream(ietf::Subscribe::ID, ietf::Body(body.into()), stream) + .unwrap() + .await; + let sent = downstream.log.datagrams.lock().unwrap(); + original.retain(|datagram| !ranged || datagram.object_id == Some(7)); + assert_eq!(sent.len(), original.len()); + for (bytes, mut expected) in sent.iter().zip(original) { + expected.track_alias = 3; + let (actual, _) = ietf::ObjectDatagram::decode_slice(bytes, version).unwrap(); + assert_eq!(actual, expected); + } + } + } + } + + #[moq_net_sim::test] + async fn passthrough_translates_mutable_property_encodings() { + for ingress in [Version::Draft14, Version::Draft15, Version::Draft17, Version::Draft22] { + let mut properties = Vec::new(); + let mut encoder = Encoder::new(&mut properties, ingress.into()); + encoder.varint(0x80u64).unwrap(); + encoder.varint(96_000u64).unwrap(); + encoder + .varint(if matches!(ingress, Version::Draft14 | Version::Draft15) { + 0x81u64 + } else { + 1u64 + }) + .unwrap(); + encoder.bytes(&[42; 130]).unwrap(); + for egress in [Version::Draft15, Version::Draft17, Version::Draft22] { + let script = object_stream(ingress, true, &[(0, &properties, None)]); + let (bytes, log) = relay_passthrough_stream(script, ingress, egress).await; + assert!(log.resets().is_empty()); + let mut decoder = Decoder::new(&bytes, egress.into()); + ietf::GroupHeader::decode(&mut decoder, egress).unwrap(); + assert_eq!(decoder.varint().unwrap(), 0); + let mut props = Decoder::new(decoder.bytes().unwrap(), egress.into()); + assert_eq!(props.varint().unwrap(), 0x80); + assert_eq!(props.varint().unwrap(), 96_000); + assert_eq!( + props.varint().unwrap(), + if egress == Version::Draft15 { 0x81 } else { 1 } + ); + assert_eq!(props.bytes().unwrap(), &[42; 130]); + assert!(props.is_empty()); + } + } + } + + #[moq_net_sim::test] + async fn passthrough_accepts_nonzero_first_objects_and_gaps() { + for version in [Version::Draft15, Version::Draft22] { + let mut script = Vec::new(); + ietf::GroupHeader { + track_alias: ALIAS, + group_id: SEQUENCE, + sub_group_id: 0, + publisher_priority: 0, + flags: ietf::GroupFlags::default(), + } + .encode(&mut Encoder::new(&mut script, version.into()), version) + .unwrap(); + for delta in [6u64, 1, 2] { + let mut encoder = Encoder::new(&mut script, version.into()); + encoder.varint(delta).unwrap(); + encoder.varint(1u64).unwrap(); + script.push(42); + } + let mut h = Harness::with_timescale(Fill::Done, vec![script], None); + h.subscriber.version = version; + let mut consumer = h.track.subscribe(None); + let mut stream = h.stream().await.with_version(version); + h.subscriber.clone().recv_group(&mut stream).await.unwrap(); + assert_eq!(read_group(&mut consumer).await, (SEQUENCE, vec![(None, vec![42]); 3])); + } + } + + /// A datagram's object number does not change its public payload view. + #[moq_net_sim::test] + async fn passthrough_accepts_datagrams_past_object_zero() { + for version in [Version::Draft15, Version::Draft22] { + let mut h = Harness::with_timescale(Fill::Done, vec![], None); + h.subscriber.version = version; + let mut consumer = h.track.subscribe(None); + let bytes = ietf::ObjectDatagram { + track_alias: ALIAS, + group_id: SEQUENCE, + object_id: Some(6), + publisher_priority: Some(128), + end_of_group: false, + properties: None, + body: ietf::DatagramBody::Payload(bytes::Bytes::from_static(b"object-six")), + } + .encode_bytes(version) + .unwrap(); + h.subscriber.recv_datagram(bytes).unwrap(); + let received = consumer.poll_recv_datagram(&kio::Waiter::noop()); + assert!( + matches!(&received, Poll::Ready(Ok(Some(data))) if data.payload == b"object-six"[..]), + "{version}: {received:?}" + ); + } } #[moq_net_sim::test] @@ -8455,9 +9099,9 @@ mod stitch_tests { ); } - /// Drafts without FIRST_OBJECT still identify a headless stream by its first ID. + /// Old drafts preserve a nonzero first Object ID without inventing missing frames. #[moq_net_sim::test] - async fn old_drafts_drop_a_headless_group_before_creating_it() { + async fn old_drafts_accept_a_nonzero_first_object() { for version in [Version::Draft14, Version::Draft16] { let scripts = [2u64, 0] .into_iter() @@ -8487,11 +9131,9 @@ mod stitch_tests { h.subscriber.version = version; let mut consumer = h.track.subscribe(None); let mut stream = h.stream().await.with_version(version); - assert!(matches!( - h.subscriber.clone().recv_group(&mut stream).await, - Err(Error::Unsupported) - )); - assert_eq!(h.track.latest(), None); + h.subscriber.clone().recv_group(&mut stream).await.unwrap(); + assert_eq!(h.track.latest(), Some(SEQUENCE)); + assert_eq!(read_group(&mut consumer).await, (SEQUENCE, vec![(None, vec![42])])); let mut stream = h.stream().await.with_version(version); h.subscriber.clone().recv_group(&mut stream).await.unwrap(); assert_eq!(read_group(&mut consumer).await, (SEQUENCE + 1, vec![(None, vec![42])])); @@ -9048,28 +9690,14 @@ mod stitch_tests { assert_eq!(frames, [b"2".to_vec(), b"3".to_vec()]); } - /// Without a head there is nothing to stitch onto, so a stream that starts part way - /// through a group is dropped and the join degrades to the next group boundary. This is - /// what a strict publisher gives a subscriber that asks for no fill. + /// A standalone subgroup tail keeps its identity while plain readers see its payload. #[moq_net_sim::test] - async fn a_tail_without_a_fill_is_dropped() { + async fn a_tail_without_a_fill_is_preserved() { let h = Harness::new(Fill::Done, vec![tail_stream(SEQUENCE, 2, &[b"tail-2"])]); let mut consumer = h.track.subscribe(None); let mut tail = h.stream().await; - - // The stream goes, not the session. - assert!(matches!( - h.subscriber.clone().recv_group(&mut tail).await, - Err(Error::Unsupported) - )); - - // Nothing usable reaches the model: the group is never offered at all. - let delivered = moq_net_sim::timeout(Duration::from_millis(50), async { - let mut group = consumer.recv_group().await.ok().flatten()?; - group.read_frame().await.ok().flatten() - }) - .await; - assert!(matches!(delivered, Err(_) | Ok(None)), "no frame is delivered"); + h.subscriber.clone().recv_group(&mut tail).await.unwrap(); + assert_eq!(read_group(&mut consumer).await.1[0].1, b"tail-2"); } /// Object IDs are absolute whatever FIRST_OBJECT says, and they start at 0, so a @@ -9127,10 +9755,9 @@ mod stitch_tests { assert!(matches!(end, Ok(None)), "the group is finished, not aborted: {end:?}"); } - /// A clear FIRST_OBJECT whose first ID is not 0 still has a hole at the front, so - /// the stream is dropped and the track resumes at the next group. + /// FIRST_OBJECT and the starting ID are independent relay metadata. #[moq_net_sim::test] - async fn a_clear_first_object_past_zero_is_dropped() { + async fn a_clear_first_object_past_zero_is_preserved() { let partial = draft18_subgroup(SEQUENCE, false, 3, &[b"tail-3"]); let whole = draft18_subgroup(SEQUENCE + 1, true, 0, &[b"next"]); assert_eq!(u64::from(partial[0]) & ietf::GroupFlags::FIRST_OBJECT_BIT, 0); @@ -9140,10 +9767,10 @@ mod stitch_tests { let mut consumer = h.track.subscribe(None); let mut partial = read_draft18(&h.session).await; - assert!(matches!( - h.subscriber.clone().recv_group(&mut partial).await, - Err(Error::Unsupported) - )); + h.subscriber.clone().recv_group(&mut partial).await.unwrap(); + let (sequence, frames) = read_group(&mut consumer).await; + assert_eq!(sequence, SEQUENCE); + assert_eq!(frames[0].1, b"tail-3"); let mut whole = read_draft18(&h.session).await; h.subscriber @@ -9374,22 +10001,24 @@ mod stitch_tests { ], ) .with_joining(JoiningFetch::Absolute { group_id: START }, FETCH, largest); - let mut consumer = h.track.subscribe(None); + let mut consumer = h + .track + .subscribe(track::Subscription::default().with_start(track::Position::group(START))); let mut fill = h.stream().await; let mut tail = h.stream().await; h.subscriber.clone().recv_fill(&mut fill).await.expect("short fetch"); - assert!(matches!( - h.subscriber.clone().recv_group(&mut tail).await, - Err(Error::Unsupported) - )); let (sequence, frames) = read_group(&mut consumer).await; assert_eq!(sequence, START); assert_eq!(frames.len(), 2, "the prefix that arrived is published"); assert_eq!(frames[0].1, b"g7-0"); assert_eq!(frames[1].1, b"g7-1"); + h.subscriber.clone().recv_group(&mut tail).await.unwrap(); + let (sequence, frames) = read_group(&mut consumer).await; + assert_eq!(sequence, 10); + assert_eq!(frames[0].1, b"g10-2"); } /// A draft-14/15 end-of-group marker ends a group fetch, so an object after it is a diff --git a/rs/moq-net/src/lite/test_transport.rs b/rs/moq-net/src/lite/test_transport.rs index b8f3f24dead..77a24dfa8c6 100644 --- a/rs/moq-net/src/lite/test_transport.rs +++ b/rs/moq-net/src/lite/test_transport.rs @@ -39,6 +39,7 @@ impl crate::transport::Error for SinkError { /// stray second reset (a drop fallback firing behind an abort) is visible. #[derive(Clone, Default)] pub struct Log { + pub datagrams: Arc>>, pub writes: Arc>>, pub resets: Arc>>, /// Every write, FIN and reset on every send stream, in call order. See [`Self::trail`]. @@ -659,6 +660,7 @@ pub struct ScriptedSession { /// in order. See [`Self::with_incoming_unis`]. incoming_unis: Arc>>>, incoming_bidis: Arc>>>, + datagram_size: usize, } impl ScriptedSession { @@ -667,6 +669,11 @@ impl ScriptedSession { self } + pub fn with_datagrams(mut self, max_size: usize) -> Self { + self.datagram_size = max_size; + self + } + pub fn new(script: Vec) -> Self { Self { log: Log::default(), @@ -678,6 +685,7 @@ impl ScriptedSession { park: kio::Park::default(), incoming_unis: Arc::new(Mutex::new(std::collections::VecDeque::new())), incoming_bidis: Arc::new(Mutex::new(std::collections::VecDeque::new())), + datagram_size: 0, } } @@ -819,7 +827,12 @@ impl poll::Session for ScriptedSession { Poll::Ready(Ok(SinkSend::new(self.log.clone()))) } - fn poll_send_datagram(&mut self, _cx: &mut Context<'_>, _payload: &[u8]) -> Poll> { + fn poll_send_datagram(&mut self, _cx: &mut Context<'_>, payload: &[u8]) -> Poll> { + self.log + .datagrams + .lock() + .unwrap() + .push(bytes::Bytes::copy_from_slice(payload)); Poll::Ready(Ok(())) } @@ -828,7 +841,7 @@ impl poll::Session for ScriptedSession { } fn max_datagram_size(&self) -> usize { - 0 + self.datagram_size } fn binding(&self) -> crate::transport::Binding { diff --git a/rs/moq-net/src/model/datagram.rs b/rs/moq-net/src/model/datagram.rs index 04fca1e78a4..e97796b4829 100644 --- a/rs/moq-net/src/model/datagram.rs +++ b/rs/moq-net/src/model/datagram.rs @@ -41,3 +41,31 @@ pub struct Datagram { /// The datagram payload. pub payload: Bytes, } + +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct Wire { + pub object_id: Option, + pub priority: Option, + pub end_of_group: bool, + pub status: Option, + pub properties: super::object::Properties, +} + +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct Stored { + pub datagram: Datagram, + pub wire: Option>, +} + +impl From for Stored { + fn from(datagram: Datagram) -> Self { + Self { datagram, wire: None } + } +} + +impl std::ops::Deref for Stored { + type Target = Datagram; + fn deref(&self) -> &Datagram { + &self.datagram + } +} diff --git a/rs/moq-net/src/model/frame.rs b/rs/moq-net/src/model/frame.rs index d4b6deeb89f..6f3577768eb 100644 --- a/rs/moq-net/src/model/frame.rs +++ b/rs/moq-net/src/model/frame.rs @@ -48,6 +48,38 @@ pub struct Frame { pub payload: Bytes, } +/// Cached frames retain relay metadata without changing public struct literals. +#[derive(Clone, Debug)] +pub(crate) struct Stored { + pub frame: Frame, + pub fields: Option>, +} + +impl From for Stored { + fn from(frame: Frame) -> Self { + Self { frame, fields: None } + } +} + +impl std::ops::Deref for Stored { + type Target = Frame; + fn deref(&self) -> &Self::Target { + &self.frame + } +} + +/// A streamed frame's private header, alongside the unchanged public header. +pub(crate) struct Header { + pub info: Info, + pub fields: Option>, +} + +impl From for Header { + fn from(info: Info) -> Self { + Self { info, fields: None } + } +} + /// A reusable batch of frames, filled by [`group::Consumer::poll_read_frames`] and drained /// by [`group::Producer::write_frames`]. /// @@ -684,6 +716,7 @@ pub struct Consumer { // The group's channel, used to park while a partial frame fills. state: kio::Consumer, info: Info, + pub(crate) fields: Option>, source: Source, // Byte offset consumed so far. read_idx: usize, @@ -707,10 +740,12 @@ impl std::ops::Deref for Consumer { } impl Consumer { - pub(crate) fn new(state: kio::Consumer, info: Info, source: Source) -> Self { + pub(crate) fn new(state: kio::Consumer, header: impl Into
, source: Source) -> Self { + let header = header.into(); Self { state, - info, + info: header.info, + fields: header.fields, source, read_idx: 0, stats: stats::Meter::default(), @@ -751,6 +786,9 @@ impl Consumer { if !recover.wants(failed.as_ref(), waiter) { return res; } + if self.state.read().wire.as_ref().is_some_and(|wire| wire.sparse) { + return Poll::Ready(Err(Error::Unsupported)); + } let mut replacement = match recover.poll(*index, failed.as_ref(), waiter) { Poll::Ready(Ok(group)) => group, Poll::Ready(Err(err)) => return Poll::Ready(Err(err)), diff --git a/rs/moq-net/src/model/group.rs b/rs/moq-net/src/model/group.rs index 96f112b969a..3da8f4131db 100644 --- a/rs/moq-net/src/model/group.rs +++ b/rs/moq-net/src/model/group.rs @@ -57,7 +57,7 @@ pub(crate) const CACHE_OVERHEAD: u64 = (kio::Producer::::HEAP // straight after. + 2 * size_of::() + size_of::() - + FRAME_SLOTS * size_of::()) as u64; + + FRAME_SLOTS * size_of::()) as u64; /// A group contains a sequence number because they can arrive out of order. /// @@ -125,6 +125,7 @@ impl From for Info { /// group is a single ordered stream. pub(crate) struct Partial { timestamp: Option, + fields: Option>, buf: FrameBuf, // How much of `buf` has been charged to the cache so far. charged: u64, @@ -136,7 +137,11 @@ pub(crate) struct Partial { pub(crate) struct GroupState { // Completed frames, each a contiguous payload. `offset` is the first frame this // handle holds, raised by [`Producer::start_at`]. - pub(crate) frames: VecDeque, + pub(crate) frames: VecDeque, + // Known before frames arrive; the terminal object is learned at the stream tail. + pub(crate) wire: Option, + pub(crate) end_object: Option>, + metadata_bytes: u64, // The single in-flight frame, if one is open. pub(crate) partial: Option, @@ -182,6 +187,19 @@ pub(crate) struct GroupState { pub(crate) abort: Option, } +/// The IETF header state that cannot be inferred from a plain frame list. +#[derive(Clone, Debug)] +pub(crate) struct Wire { + /// Object IDs differ from the public frame ordinals, so ordinal recovery cannot stitch. + pub sparse: bool, + pub first_object: bool, + pub first_id: u64, + pub extensions: bool, + pub has_end: bool, + pub fetchable: bool, + pub last: Option, +} + /// Where a group sits in presentation time. /// /// Its first frame settles which: an empty group has not presented anything yet, which @@ -262,7 +280,7 @@ impl GroupState { /// Resolve the source for the frame at `index`: a completed frame (whole) or the /// in-flight tail (streamed). Used by [`Consumer::poll_next_frame`]. - fn poll_frame_source(&self, index: usize) -> Poll>> { + fn poll_frame_source(&self, index: usize) -> Poll>> { if index < self.offset { return Poll::Ready(Err(Error::Lagged)); } @@ -275,7 +293,13 @@ impl GroupState { size: f.payload.len() as u64, timestamp: f.timestamp, }; - return Poll::Ready(Ok(Some((info, frame::Source::Complete(f.payload.clone()))))); + return Poll::Ready(Ok(Some(( + frame::Header { + info, + fields: f.fields.clone(), + }, + frame::Source::Complete(f.payload.clone()), + )))); } if local == self.frames.len() && let Some(p) = &self.partial @@ -285,7 +309,13 @@ impl GroupState { size: p.buf.size() as u64, timestamp: p.timestamp, }; - return Poll::Ready(Ok(Some((info, frame::Source::Partial(p.buf.clone()))))); + return Poll::Ready(Ok(Some(( + frame::Header { + info, + fields: p.fields.clone(), + }, + frame::Source::Partial(p.buf.clone()), + )))); } ready!(self.poll_terminal(index))?; Poll::Ready(Ok(None)) @@ -329,7 +359,11 @@ impl GroupState { /// Whether adding `extra_frames` totaling `extra_bytes` would exceed the group budget. fn would_overflow(&self, extra_frames: usize, extra_bytes: u64) -> bool { self.next_index.saturating_sub(self.offset).saturating_add(extra_frames) > MAX_GROUP_FRAMES - || self.cache.saturating_add(extra_bytes) > MAX_CACHE_BYTES + || self + .cache + .saturating_add(self.metadata_bytes) + .saturating_add(extra_bytes) + > MAX_CACHE_BYTES } /// Reserve the next completed frame, returning growth not covered by the fixed overhead. @@ -337,7 +371,7 @@ impl GroupState { let before = self.frames.capacity().saturating_sub(FRAME_SLOTS); self.frames.reserve(1); let after = self.frames.capacity().saturating_sub(FRAME_SLOTS); - ((after - before) * size_of::()) as u64 + ((after - before) * size_of::()) as u64 } /// Charge the in-flight frame's bytes written since its last charge, as a write access. @@ -364,8 +398,10 @@ impl GroupState { fn release(&mut self) { self.frames = VecDeque::new(); self.partial = None; + self.metadata_bytes = self.end_object.as_ref().map_or(0, |fields| fields.heap()); self.cache = 0; self.charge.clear(); + self.charge.add(self.metadata_bytes); } } @@ -485,6 +521,55 @@ impl std::ops::Deref for Producer { } impl Producer { + pub(crate) fn wire(&self) -> Option { + self.state.read().wire.clone() + } + + pub(crate) fn set_wire(&self, wire: Wire) -> Result<()> { + modify(&self.state)?.wire = Some(wire); + Ok(()) + } + + pub(crate) fn mark_gap(&self, id: u64, gap: Option) -> Result<()> { + if gap.is_some_and(|gap| gap > id) { + return Err(Error::MalformedTrack); + } + let mut state = modify(&self.state)?; + let offset = state.offset as u64; + if let Some(wire) = &mut state.wire { + let missing = match wire.last { + None => id.checked_sub(offset).ok_or(Error::MalformedTrack)?, + Some(prior) => id + .checked_sub(prior) + .and_then(|step| step.checked_sub(1)) + .ok_or(Error::MalformedTrack)?, + }; + if wire.last.is_some() && gap.is_some_and(|gap| gap > missing) { + return Err(Error::MalformedTrack); + } + wire.fetchable &= missing == 0 || gap == Some(missing); + } + Ok(()) + } + + pub(crate) fn set_end_object(&self, fields: super::object::Fields) -> Result<()> { + let mut state = modify(&self.state)?; + if state.fin.is_some() || state.end_object.is_some() { + return Err(Error::MalformedTrack); + } + let bytes = fields.heap(); + if state.would_overflow(0, bytes) { + return Err(Error::GroupTooLarge); + } + state.charge.add(bytes); + state.metadata_bytes += bytes; + if let Some(wire) = &mut state.wire { + wire.last = Some(fields.id); + } + state.end_object = Some(Arc::new(fields)); + Ok(()) + } + /// Create a group producer bound to its parent track's [`track::Info`] and cache /// account. /// @@ -599,7 +684,7 @@ impl Producer { state.cache += size; let slots = state.reserve_frame(); let now = state.charge.add(size + slots); - state.frames.push_back(Frame { timestamp, payload }); + state.frames.push_back(Frame { timestamp, payload }.into()); state.next_index = next_index; state.committed = state.next_index; state.stamp(timestamp); @@ -666,7 +751,7 @@ impl Producer { now = state.charge.add(size + slots); state.stamp(frame.timestamp); latest = frame.timestamp; - state.frames.push_back(frame); + state.frames.push_back(frame.into()); } state.next_index = next_index; state.committed = next_index; @@ -692,7 +777,7 @@ impl Producer { pub fn create_frame(&mut self, frame: frame::Info) -> Result> { // The buffer allocates on its first write, so refusing the frame costs nothing. let buf = FrameBuf::new(frame.size as usize); - let info = self.open_frame(frame, &buf)?; + let info = self.open_frame(frame.into(), &buf)?; let meter = self.stats.clone(); Ok(frame::Producer::new(self, buf, info).with_meter(meter)) } @@ -707,10 +792,11 @@ impl Producer { #[cfg_attr(not(feature = "async"), allow(dead_code))] pub(crate) fn create_frame_owned( &mut self, - frame: frame::Info, + frame: impl Into, budget: &frame::Budget, ) -> Result { - let size = frame.size as usize; + let frame = frame.into(); + let size = frame.info.size as usize; let reserved = budget.reserve(size); let buf = match reserved { Some(_) => FrameBuf::new(size), @@ -725,7 +811,8 @@ impl Producer { /// /// Its bytes are charged to the cache as they are written, not here: the declared /// size is only a promise until they arrive. - fn open_frame(&mut self, frame: frame::Info, buf: &FrameBuf) -> Result { + fn open_frame(&mut self, header: frame::Header, buf: &FrameBuf) -> Result { + let frame = header.info; let timestamp = on_track(frame.timestamp, self.track.timescale)?; if frame.size > MAX_CACHE_BYTES { return Err(Error::FrameTooLarge); @@ -745,15 +832,28 @@ impl Producer { state.admit(next_index)?; // Only one frame is ever in flight, so the declared size is the most the cache // can grow by before the next check. - if state.would_overflow(1, frame.size) { + let metadata_bytes = header.fields.as_ref().map_or(0, |fields| fields.heap()); + if state.would_overflow(1, frame.size.saturating_add(metadata_bytes)) { return Err(self.abort_too_large(state)); } let now = state.charge.record_write(); state.partial = Some(Partial { timestamp, + fields: header.fields, buf: buf.clone(), charged: 0, }); + state.metadata_bytes += metadata_bytes; + state.charge.add(metadata_bytes); + let object_id = state + .partial + .as_ref() + .and_then(|partial| partial.fields.as_ref()) + .map_or(state.next_index as u64, |fields| fields.id); + if let Some(wire) = &mut state.wire { + wire.last = Some(object_id); + wire.sparse |= object_id != (next_index - 1) as u64; + } state.next_index = next_index; // Opening the frame is enough: the header carries the timestamp, so the group's // place in time is known before a single payload byte streams in. @@ -801,8 +901,8 @@ impl Producer { // Without this, a group whose payload streamed in across an idle gap would expire // the instant it finished. let now = state.charge_partial(); - state.partial = None; - state.frames.push_back(frame); + let fields = state.partial.take().and_then(|partial| partial.fields); + state.frames.push_back(frame::Stored { frame, fields }); state.committed = state.next_index; drop(state); @@ -945,7 +1045,7 @@ impl Producer { /// tail, and the count outlives the released cache. pub(crate) fn resume_frame(&self) -> Option { let state = self.state.read(); - if state.fin.is_some() { + if state.fin.is_some() || state.wire.as_ref().is_some_and(|wire| wire.sparse) { return None; } (state.committed > state.offset).then_some(state.committed) @@ -1217,7 +1317,7 @@ impl Clone for Producer { /// reads whole frames, or drains through a higher-level buffer, pays nothing. struct Prefetch { // Initialized, not-yet-taken frames are `frames[pos..len]`; the rest are uninitialized. - frames: [MaybeUninit; Self::CAP], + frames: [MaybeUninit; Self::CAP], pos: usize, len: usize, } @@ -1226,7 +1326,7 @@ impl Prefetch { const CAP: usize = 8; /// Take the next buffered frame, or `None` if the batch is drained. - fn pop(&mut self) -> Option { + fn pop(&mut self) -> Option { if self.pos == self.len { return None; } @@ -1237,7 +1337,7 @@ impl Prefetch { } /// Refill with up to `CAP` frames. Must be drained first (`pop` returned `None`). - fn fill(&mut self, frames: impl Iterator) { + fn fill(&mut self, frames: impl Iterator) { debug_assert_eq!(self.pos, self.len, "fill on a non-empty batch would leak frames"); self.pos = 0; self.len = 0; @@ -1388,6 +1488,42 @@ impl std::ops::Deref for Consumer { } impl Consumer { + pub(crate) fn last_object(&self) -> Option { + let state = self.cursor.state.read(); + match &state.wire { + Some(wire) => wire.last, + None => (state.next_index as u64).checked_sub(1), + } + } + + pub(crate) fn wire(&self) -> Option { + self.cursor.state.read().wire.clone() + } + + pub(crate) fn end_object(&self) -> Option> { + self.cursor.state.read().end_object.clone() + } + + /// Fill a private batch without losing metadata through the public frame view. + pub(crate) fn poll_read_objects( + &mut self, + waiter: &kio::Waiter, + out: &mut arrayvec::ArrayVec, + ) -> Poll> { + out.clear(); + while !out.is_full() { + match self.poll_read_object(waiter) { + Poll::Ready(Ok(Some(frame))) => out.push(frame), + Poll::Ready(Ok(None)) => break, + Poll::Ready(Err(err)) if out.is_empty() => return Poll::Ready(Err(err)), + Poll::Ready(Err(_)) => break, + Poll::Pending if out.is_empty() => return Poll::Pending, + Poll::Pending => break, + } + } + Poll::Ready(Ok(out.len())) + } + /// Snapshot the content this cursor would discard if its group were skipped. pub(crate) fn content(&self) -> stats::Content { self.cursor.state.read().content() @@ -1442,6 +1578,10 @@ impl Consumer { if !recover.wants(failed.as_ref(), waiter) { return res; } + // An ordinal FETCH would request different content after an Object ID gap. + if self.cursor.state.read().wire.as_ref().is_some_and(|wire| wire.sparse) { + return Poll::Ready(Err(Error::Unsupported)); + } let replacement = match recover.poll(self.cursor.index as u64, failed.as_ref(), waiter) { Poll::Ready(Ok(group)) => group, Poll::Ready(Err(err)) => return Poll::Ready(Err(err)), @@ -1665,13 +1805,18 @@ impl Consumer { /// Read the next frame (timestamp and payload) all at once, without blocking. pub fn poll_read_frame(&mut self, waiter: &kio::Waiter) -> Poll>> { + self.poll_read_object(waiter) + .map(|res| res.map(|frame| frame.map(|frame| frame.frame))) + } + + pub(crate) fn poll_read_object(&mut self, waiter: &kio::Waiter) -> Poll>> { if self.recover.is_none() { return self.poll_read_frame_once(waiter); } self.poll_resumed(waiter, Self::poll_read_frame_once) } - fn poll_read_frame_once(&mut self, waiter: &kio::Waiter) -> Poll>> { + fn poll_read_frame_once(&mut self, waiter: &kio::Waiter) -> Poll>> { if self.ended { return Poll::Ready(Ok(None)); } @@ -1698,7 +1843,7 @@ impl Consumer { { self.cursor.refresh_if_stale(); self.cursor.index += 1; - return Ok(Some(frame)); + return Ok(Some(frame.frame)); } kio::wait(|waiter| self.poll_read_frame(waiter)).await } @@ -1878,8 +2023,12 @@ impl Cursor { size: frame.payload.len() as u64, timestamp: frame.timestamp, }; - let source = frame::Source::Complete(frame.payload); - let frame = frame::Consumer::new(self.state.clone(), info, source); + let source = frame::Source::Complete(frame.frame.payload); + let header = frame::Header { + info, + fields: frame.fields, + }; + let frame = frame::Consumer::new(self.state.clone(), header, source); return Poll::Ready(Ok(Some(match expiry { Some(expiry) => frame.with_expiry(expiry.for_frame(tail, false)), None => frame, @@ -1902,7 +2051,7 @@ impl Cursor { }))) } - fn poll_read_frame(&mut self, waiter: &kio::Waiter, stats: &stats::Meter) -> Poll>> { + fn poll_read_frame(&mut self, waiter: &kio::Waiter, stats: &stats::Meter) -> Poll>> { if self.capped() { return Poll::Ready(Ok(None)); } @@ -2025,6 +2174,28 @@ mod test { #[cfg(feature = "async")] use futures::FutureExt; + #[test] + fn terminal_metadata_stays_charged_after_frames_are_released() { + let pool = cache::Pool::unbounded(); + let cache = cache::Track::new(pool.clone(), kio::Weak::new()); + let mut group = Producer::new(Info::from(0u64), track::Info::default(), cache); + group.write_frame(None, Bytes::from_static(b"payload")).unwrap(); + let fields = super::super::object::Fields { + id: 4, + properties: Default::default(), + }; + let charged = fields.heap(); + group.set_end_object(fields.clone()).unwrap(); + assert!(matches!(group.set_end_object(fields), Err(Error::MalformedTrack))); + group.finish().unwrap(); + let consumer = group.consume(); + group.abort(Error::Cancel).unwrap(); + assert_eq!(consumer.end_object().unwrap().id, 4); + assert_eq!(pool.used(), charged); + drop(consumer); + assert_eq!(pool.used(), 0); + } + #[test] #[cfg(feature = "async")] fn demand_watches_readers_without_keeping_the_group_alive() { @@ -2080,7 +2251,7 @@ mod test { assert!(capacity > FRAME_SLOTS); assert_eq!( pool.used() - before, - ((capacity - FRAME_SLOTS) * size_of::()) as u64 + ((capacity - FRAME_SLOTS) * size_of::()) as u64 ); let consumer = group.consume(); group.abort(Error::Cancel).unwrap(); @@ -2092,11 +2263,14 @@ mod test { /// would undercharge every cached group without touching a line of this crate. #[test] fn one_frame_fits_the_charged_slots() { - let mut frames: VecDeque = VecDeque::new(); - frames.push_back(Frame { - timestamp: Some(Timestamp::ZERO), - payload: Bytes::new(), - }); + let mut frames: VecDeque = VecDeque::new(); + frames.push_back( + Frame { + timestamp: Some(Timestamp::ZERO), + payload: Bytes::new(), + } + .into(), + ); let capacity = frames.capacity(); assert!( capacity <= FRAME_SLOTS, diff --git a/rs/moq-net/src/model/mod.rs b/rs/moq-net/src/model/mod.rs index a26b558c69f..94c1aba0d0b 100644 --- a/rs/moq-net/src/model/mod.rs +++ b/rs/moq-net/src/model/mod.rs @@ -18,9 +18,10 @@ mod front; mod bytes; pub(crate) mod clock; -mod datagram; +pub(crate) mod datagram; mod delivered; mod expiry; +pub(crate) mod object; mod requests; mod resume; mod subscription; diff --git a/rs/moq-net/src/model/object.rs b/rs/moq-net/src/model/object.rs new file mode 100644 index 00000000000..077e93b0bf5 --- /dev/null +++ b/rs/moq-net/src/model/object.rs @@ -0,0 +1,61 @@ +//! Private relay metadata; public readers see only the frame or datagram payload. +use bytes::Bytes; + +/// How the publisher serialized the keys inside immutable properties. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum Encoding { + Absolute, + DeltaQuic, + Delta { seven: bool }, +} + +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) enum Value { + Int(u64), + Bytes(Bytes), +} + +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct Property { + pub id: u64, + pub value: Value, +} + +/// The common timestamp needs no vector allocation; all other properties stay opaque. +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub(crate) struct Properties { + pub timestamp: Option, + pub values: Vec, + pub immutable: Option, +} + +impl Properties { + pub fn is_plain(&self) -> bool { + self.values.is_empty() + } + + pub fn heap(&self) -> u64 { + (self.values.capacity() * size_of::()) as u64 + + self + .values + .iter() + .map(|property| match &property.value { + Value::Bytes(bytes) => bytes.len() as u64, + Value::Int(_) => 0, + }) + .sum::() + } +} + +/// Object numbering is separate from the public frame's ordinal in its group. +#[derive(Clone, Debug)] +pub(crate) struct Fields { + pub id: u64, + pub properties: Properties, +} + +impl Fields { + pub fn heap(&self) -> u64 { + (size_of::() + 2 * size_of::()) as u64 + self.properties.heap() + } +} diff --git a/rs/moq-net/src/model/resume.rs b/rs/moq-net/src/model/resume.rs index ccb25b0bbc6..98830fc7e23 100644 --- a/rs/moq-net/src/model/resume.rs +++ b/rs/moq-net/src/model/resume.rs @@ -21,7 +21,7 @@ use std::sync::{Arc, Mutex, MutexGuard, PoisonError, Weak}; use std::task::{Poll, ready}; -use crate::{Datagram, Error, Result, StreamError, group, stats, track}; +use crate::{Error, Result, StreamError, group, stats, track}; use super::delivered::Delivered; use super::subscription::{Cap, Position, Subscription, max_some}; @@ -341,7 +341,7 @@ impl Subscriber { &mut self, delivered: &Delivered, waiter: &kio::Waiter, - ) -> Poll>> { + ) -> Poll>> { self.reader().poll_recv_datagram(delivered, waiter) } @@ -667,7 +667,11 @@ impl Reader { } } - fn poll_recv_datagram(&mut self, delivered: &Delivered, waiter: &kio::Waiter) -> Poll>> { + fn poll_recv_datagram( + &mut self, + delivered: &Delivered, + waiter: &kio::Waiter, + ) -> Poll>> { loop { if let Some(res) = self.poll_datagram_once(delivered, waiter) { return res; @@ -679,7 +683,7 @@ impl Reader { &mut self, delivered: &Delivered, waiter: &kio::Waiter, - ) -> Option>>> { + ) -> Option>>> { self.sync(waiter); self.let_go(delivered); let groups = self.groups; @@ -697,7 +701,7 @@ impl Reader { } Poll::Ready(Ok(sub)) => sub, }; - match sub.poll_recv_datagram(waiter) { + match sub.poll_recv_wire_datagram(waiter) { Poll::Pending => break, Poll::Ready(Ok(Some(datagram))) => { if copy.datagrams_seen.is_some_and(|seen| datagram.sequence <= seen) { @@ -2056,6 +2060,75 @@ mod test { assert!(matches!(reading.read_frame().now_or_never(), Some(Err(Error::Old)))); } + #[test] + fn sparse_object_ids_refuse_ordinal_recovery() { + let routes = Producer::new(); + let logical = logical(&routes); + let a = copy(); + let mut group = a.create_group(group::Info { sequence: 0 }).unwrap(); + group + .set_wire(group::Wire { + sparse: false, + first_object: true, + first_id: 6, + extensions: false, + has_end: true, + fetchable: false, + last: None, + }) + .unwrap(); + let mut frame = group + .create_frame_owned( + crate::frame::Header { + info: crate::frame::Info { + size: 1, + timestamp: Some(ts(0)), + }, + fields: Some(Arc::new(super::super::object::Fields { + id: 6, + properties: Default::default(), + })), + }, + &crate::frame::Budget::default(), + ) + .unwrap(); + frame.write(bytes::Bytes::from_static(b"a")).unwrap(); + frame.finish().unwrap(); + routes.serve(a.consume()); + let mut sub = subscribe(&logical, Duration::MAX); + let mut reading = recv(&mut sub); + assert_eq!(read(&mut reading), Some(b"a".to_vec())); + assert_eq!(a.consume().resume_position(), Some(Position::group(1))); + let mut partial = group + .create_frame_owned( + crate::frame::Header { + info: crate::frame::Info { + size: 2, + timestamp: Some(ts(1)), + }, + fields: Some(Arc::new(super::super::object::Fields { + id: 8, + properties: Default::default(), + })), + }, + &crate::frame::Budget::default(), + ) + .unwrap(); + partial.write(bytes::Bytes::from_static(b"b")).unwrap(); + let mut chunks = reading.next_frame().now_or_never().unwrap().unwrap().unwrap(); + assert_eq!(chunks.read_chunk().now_or_never().unwrap().unwrap().unwrap(), b"b"[..]); + partial.abort(Error::Dropped).unwrap(); + routes.serve(copy().consume()); + assert!(matches!( + chunks.read_chunk().now_or_never(), + Some(Err(Error::Unsupported)) + )); + assert!(matches!( + reading.read_frame().now_or_never(), + Some(Err(Error::Unsupported)) + )); + } + #[test] fn a_fetched_group_wakes_when_the_route_changes() { let routes = Producer::new(); diff --git a/rs/moq-net/src/model/track.rs b/rs/moq-net/src/model/track.rs index 4f680fd5ffc..2ae78e53c84 100644 --- a/rs/moq-net/src/model/track.rs +++ b/rs/moq-net/src/model/track.rs @@ -262,7 +262,7 @@ pub(crate) struct TrackState { // Datagrams in arrival order, bounded by `MAX_DATAGRAMS`. Shares the group // sequence namespace but is otherwise independent. - datagrams: VecDeque, + datagrams: VecDeque, // Number of datagrams dropped off the front (over capacity), mapping a subscriber's absolute // cursor to an index into `datagrams` (mirrors `offset` for groups). @@ -569,7 +569,7 @@ impl TrackState { /// Returns the datagram and its absolute index so the consumer can advance past it. A /// consumer whose `index` has fallen behind `datagram_offset` (older datagrams dropped) /// resumes at the oldest still-buffered datagram, skipping the lost ones. - fn poll_recv_datagram(&self, index: usize) -> Poll>> { + fn poll_recv_datagram(&self, index: usize) -> Poll>> { let start = index.saturating_sub(self.datagram_offset); if self.readable() && let Some(datagram) = self.datagrams.get(start) @@ -593,7 +593,7 @@ impl TrackState { } /// Push a datagram, dropping the oldest when the send buffer is full. - fn push_datagram(&mut self, datagram: Datagram) { + fn push_datagram(&mut self, datagram: super::datagram::Stored) { if self.datagrams.len() == MAX_DATAGRAMS { self.datagrams.pop_front(); self.datagram_offset += 1; @@ -1976,11 +1976,14 @@ impl Producer { } state.advance(sequence); meter.datagram(payload.len() as u64); - state.push_datagram(Datagram { - sequence, - timestamp, - payload, - }); + state.push_datagram( + Datagram { + sequence, + timestamp, + payload, + } + .into(), + ); let cache = state.cache.clone(); drop(state); cache.settle(None); @@ -1998,28 +2001,36 @@ impl Producer { timestamp: impl Into>, payload: B, ) -> Result<()> { - let payload = payload.into_bytes(); - if payload.len() > super::datagram::MAX_DATAGRAM_PAYLOAD { + self.insert_wire_datagram( + Datagram { + sequence, + timestamp: timestamp.into(), + payload: payload.into_bytes(), + } + .into(), + ) + } + + pub(crate) fn insert_wire_datagram(&mut self, mut datagram: super::datagram::Stored) -> Result<()> { + if datagram.payload.len() > super::datagram::MAX_DATAGRAM_PAYLOAD { return Err(Error::FrameTooLarge); } - // Resolved before the state guard borrows `self`. let meter = self.stats.meter(); let mut state = self.modify()?; - // Normalize into the track's timescale, like frames (see `group::Producer::create_frame`). let timescale = state.info.as_ref().unwrap().timescale; - let timestamp = group::on_track(timestamp.into(), timescale)?; - if let Some(fin) = state.final_sequence() - && sequence >= fin + if !datagram + .wire + .as_ref() + .is_some_and(|wire| wire.status.is_some_and(|status| status != 0)) { + datagram.datagram.timestamp = group::on_track(datagram.timestamp, timescale)?; + } + if state.final_sequence().is_some_and(|fin| datagram.sequence >= fin) { return Err(Error::Closed); } - state.advance(sequence); - meter.datagram(payload.len() as u64); - state.push_datagram(Datagram { - sequence, - timestamp, - payload, - }); + state.advance(datagram.sequence); + meter.datagram(datagram.payload.len() as u64); + state.push_datagram(datagram); let cache = state.cache.clone(); drop(state); cache.settle(None); @@ -3163,6 +3174,9 @@ impl Consumer { /// miss. #[cfg_attr(not(feature = "async"), allow(dead_code))] pub(crate) fn peek_group(&self, sequence: u64) -> Option { + if let Some(serving) = self.serving() { + return serving.peek_group(sequence); + } let state = self.state.read(); let group = state.lookup.get(&sequence)?.serving(); if group.is_aborted() { @@ -4165,11 +4179,11 @@ impl Cursor { /// The next datagram inside this subscriber's range, dropping the ones outside it. /// - /// The range is the one groups get: the floor, the cap, and a start frame past 0, which - /// excludes the start group's only frame. A datagram is judged when the cursor reaches + /// The range is the one groups get: the floor, the cap, and the start Object ID. + /// Plain datagrams have Object ID 0. A datagram is judged when the cursor reaches /// it, against the range in force then, and one outside it is gone for good: unlike a /// group past the cap, nothing holds it for a later raise. - fn poll_recv_datagram(&mut self, waiter: &kio::Waiter) -> Poll>> { + fn poll_recv_datagram(&mut self, waiter: &kio::Waiter) -> Poll>> { loop { let Some((datagram, found_index)) = ready!(self.poll(waiter, |state| state.poll_recv_datagram(self.datagram_index))?) @@ -4179,11 +4193,10 @@ impl Cursor { self.datagram_index = found_index + 1; let sequence = datagram.sequence; - let mid_group = self - .subscription - .read() - .start - .is_some_and(|start| start.group == sequence && start.frame > 0); + let mid_group = self.subscription.read().start.is_some_and(|start| { + start.group == sequence + && start.frame > datagram.wire.as_ref().and_then(|wire| wire.object_id).unwrap_or(0) + }); if sequence >= self.min_sequence && super::subscription::before_end(sequence, self.end_sequence) && !mid_group @@ -4475,6 +4488,25 @@ impl Subscriber { /// `Poll::Ready(Ok(None))` when the track is finished, `Poll::Ready(Err(e))` when the track /// is aborted, or `Poll::Pending` when none is buffered yet. pub fn poll_recv_datagram(&mut self, waiter: &kio::Waiter) -> Poll>> { + loop { + let Some(datagram) = ready!(self.poll_recv_wire_datagram(waiter))? else { + return Poll::Ready(Ok(None)); + }; + if datagram + .wire + .as_ref() + .is_some_and(|wire| wire.status.is_some_and(|status| status != 0)) + { + continue; + } + return Poll::Ready(Ok(Some(datagram.datagram))); + } + } + + pub(crate) fn poll_recv_wire_datagram( + &mut self, + waiter: &kio::Waiter, + ) -> Poll>> { let meter = self.stats.meter(); let res = match &mut self.inner { Inner::Plain(cursor) => cursor.poll_recv_datagram(waiter), From 5bbb613520fcb76393d21823c7c5061f8836b429 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 14 Oct 2026 10:13:08 -0700 Subject: [PATCH 03/12] perf(net): keep relay object fields off the plain read path Co-Authored-By: Claude Opus 5.5 --- rs/moq-net/src/ietf/publisher.rs | 6 +- rs/moq-net/src/model/frame.rs | 12 +- rs/moq-net/src/model/group.rs | 189 ++++++++++++++++++++++++------- 3 files changed, 156 insertions(+), 51 deletions(-) diff --git a/rs/moq-net/src/ietf/publisher.rs b/rs/moq-net/src/ietf/publisher.rs index 9d1175c734d..7498dd970bd 100644 --- a/rs/moq-net/src/ietf/publisher.rs +++ b/rs/moq-net/src/ietf/publisher.rs @@ -79,7 +79,7 @@ struct FetchedGroup { sequence: u64, /// The Object ID of the first frame. first: u64, - frames: Vec, + frames: Vec, /// The fetched range reached the finished group's final object. complete: bool, /// The track's info, which FETCH_OK describes and whose units stamp the objects. @@ -1315,7 +1315,7 @@ where let mut index: u64 = 0; let mut prior = None; - let mut buf = arrayvec::ArrayVec::::new(); + let mut buf = arrayvec::ArrayVec::::new(); if group.wire().is_some_and(|wire| !wire.fetchable) { return Err(Error::Unsupported); } @@ -3543,7 +3543,7 @@ enum GroupState { writer: Writer, frame: Option, chunk: Option, - batch: Box>, + batch: Box>, batch_pos: usize, }, /// Every frame is written and the FIN sent: wait for the acknowledgement so a diff --git a/rs/moq-net/src/model/frame.rs b/rs/moq-net/src/model/frame.rs index 6f3577768eb..2b7b2359068 100644 --- a/rs/moq-net/src/model/frame.rs +++ b/rs/moq-net/src/model/frame.rs @@ -48,20 +48,14 @@ pub struct Frame { pub payload: Bytes, } -/// Cached frames retain relay metadata without changing public struct literals. +/// A frame with the relay metadata its group keeps beside it, read only by wire egress. #[derive(Clone, Debug)] -pub(crate) struct Stored { +pub(crate) struct Object { pub frame: Frame, pub fields: Option>, } -impl From for Stored { - fn from(frame: Frame) -> Self { - Self { frame, fields: None } - } -} - -impl std::ops::Deref for Stored { +impl std::ops::Deref for Object { type Target = Frame; fn deref(&self) -> &Self::Target { &self.frame diff --git a/rs/moq-net/src/model/group.rs b/rs/moq-net/src/model/group.rs index 3da8f4131db..40850b3d8d9 100644 --- a/rs/moq-net/src/model/group.rs +++ b/rs/moq-net/src/model/group.rs @@ -57,7 +57,7 @@ pub(crate) const CACHE_OVERHEAD: u64 = (kio::Producer::::HEAP // straight after. + 2 * size_of::() + size_of::() - + FRAME_SLOTS * size_of::()) as u64; + + FRAME_SLOTS * size_of::()) as u64; /// A group contains a sequence number because they can arrive out of order. /// @@ -137,7 +137,10 @@ pub(crate) struct Partial { pub(crate) struct GroupState { // Completed frames, each a contiguous payload. `offset` is the first frame this // handle holds, raised by [`Producer::start_at`]. - pub(crate) frames: VecDeque, + pub(crate) frames: VecDeque, + // Relay metadata for `frames`, at the same local index. Frames past its end carry + // none, so a group that never sees a field never allocates it or pays to skip it. + objects: VecDeque>>, // Known before frames arrive; the terminal object is learned at the stream tail. pub(crate) wire: Option, pub(crate) end_object: Option>, @@ -296,7 +299,7 @@ impl GroupState { return Poll::Ready(Ok(Some(( frame::Header { info, - fields: f.fields.clone(), + fields: self.fields(local), }, frame::Source::Complete(f.payload.clone()), )))); @@ -371,7 +374,20 @@ impl GroupState { let before = self.frames.capacity().saturating_sub(FRAME_SLOTS); self.frames.reserve(1); let after = self.frames.capacity().saturating_sub(FRAME_SLOTS); - ((after - before) * size_of::()) as u64 + ((after - before) * size_of::()) as u64 + } + + /// Reserve a metadata slot for the next completed frame, returning the growth. + fn reserve_fields(&mut self) -> u64 { + let before = self.objects.capacity(); + self.objects.resize(self.frames.len(), None); + self.objects.reserve(1); + ((self.objects.capacity() - before) * size_of::>>()) as u64 + } + + /// The relay metadata of the completed frame at `local`, if it carried any. + fn fields(&self, local: usize) -> Option> { + self.objects.get(local).cloned().flatten() } /// Charge the in-flight frame's bytes written since its last charge, as a write access. @@ -386,10 +402,10 @@ impl GroupState { None => 0, }; self.cache += written; - let slots = if self.partial.is_some() { - self.reserve_frame() - } else { - 0 + let slots = match &self.partial { + Some(partial) if partial.fields.is_some() => self.reserve_frame() + self.reserve_fields(), + Some(_) => self.reserve_frame(), + None => 0, }; self.charge.add(written + slots) } @@ -397,6 +413,7 @@ impl GroupState { /// Drop the cached frames (and any in-flight tail) and release their pool charge. fn release(&mut self) { self.frames = VecDeque::new(); + self.objects = VecDeque::new(); self.partial = None; self.metadata_bytes = self.end_object.as_ref().map_or(0, |fields| fields.heap()); self.cache = 0; @@ -684,7 +701,7 @@ impl Producer { state.cache += size; let slots = state.reserve_frame(); let now = state.charge.add(size + slots); - state.frames.push_back(Frame { timestamp, payload }.into()); + state.frames.push_back(Frame { timestamp, payload }); state.next_index = next_index; state.committed = state.next_index; state.stamp(timestamp); @@ -751,7 +768,7 @@ impl Producer { now = state.charge.add(size + slots); state.stamp(frame.timestamp); latest = frame.timestamp; - state.frames.push_back(frame.into()); + state.frames.push_back(frame); } state.next_index = next_index; state.committed = next_index; @@ -901,8 +918,13 @@ impl Producer { // Without this, a group whose payload streamed in across an idle gap would expire // the instant it finished. let now = state.charge_partial(); - let fields = state.partial.take().and_then(|partial| partial.fields); - state.frames.push_back(frame::Stored { frame, fields }); + if let Some(fields) = state.partial.take().and_then(|partial| partial.fields) { + // The slot was reserved and charged by `charge_partial` above. + let len = state.frames.len(); + state.objects.resize(len, None); + state.objects.push_back(Some(fields)); + } + state.frames.push_back(frame); state.committed = state.next_index; drop(state); @@ -1317,7 +1339,9 @@ impl Clone for Producer { /// reads whole frames, or drains through a higher-level buffer, pays nothing. struct Prefetch { // Initialized, not-yet-taken frames are `frames[pos..len]`; the rest are uninitialized. - frames: [MaybeUninit; Self::CAP], + frames: [MaybeUninit; Self::CAP], + // Relay metadata for `frames`, allocated by the first batch that carries any. + fields: Option>; Self::CAP]>>, pos: usize, len: usize, } @@ -1326,7 +1350,7 @@ impl Prefetch { const CAP: usize = 8; /// Take the next buffered frame, or `None` if the batch is drained. - fn pop(&mut self) -> Option { + fn pop(&mut self) -> Option { if self.pos == self.len { return None; } @@ -1336,15 +1360,30 @@ impl Prefetch { Some(frame) } - /// Refill with up to `CAP` frames. Must be drained first (`pop` returned `None`). - fn fill(&mut self, frames: impl Iterator) { + /// The relay metadata of the frame [`Self::pop`] just returned. + fn take_fields(&mut self) -> Option> { + self.fields.as_mut()?.get_mut(self.pos.checked_sub(1)?)?.take() + } + + /// Refill with up to `budget` of the completed frames from `local` on. Must be + /// drained first (`pop` returned `None`). + fn fill(&mut self, state: &GroupState, local: usize, budget: usize) { debug_assert_eq!(self.pos, self.len, "fill on a non-empty batch would leak frames"); self.pos = 0; self.len = 0; - for frame in frames.take(Self::CAP) { - self.frames[self.len].write(frame); + for frame in state.frames.range(local..).take(budget.min(Self::CAP)) { + self.frames[self.len].write(frame.clone()); self.len += 1; } + // Plain frames never reach the metadata table, so they skip this entirely. + if local < state.objects.len() { + let fields = self.fields.get_or_insert_default(); + for (i, slot) in fields.iter_mut().enumerate() { + *slot = if i < self.len { state.fields(local + i) } else { None }; + } + } else if let Some(fields) = &mut self.fields { + fields.fill(None); + } } /// `(frame count, total payload bytes)` of the buffered, not-yet-taken frames. @@ -1363,6 +1402,7 @@ impl Default for Prefetch { fn default() -> Self { Self { frames: [const { MaybeUninit::uninit() }; Self::CAP], + fields: None, pos: 0, len: 0, } @@ -1508,7 +1548,7 @@ impl Consumer { pub(crate) fn poll_read_objects( &mut self, waiter: &kio::Waiter, - out: &mut arrayvec::ArrayVec, + out: &mut arrayvec::ArrayVec, ) -> Poll> { out.clear(); while !out.is_full() { @@ -1805,18 +1845,23 @@ impl Consumer { /// Read the next frame (timestamp and payload) all at once, without blocking. pub fn poll_read_frame(&mut self, waiter: &kio::Waiter) -> Poll>> { - self.poll_read_object(waiter) - .map(|res| res.map(|frame| frame.map(|frame| frame.frame))) - } - - pub(crate) fn poll_read_object(&mut self, waiter: &kio::Waiter) -> Poll>> { if self.recover.is_none() { return self.poll_read_frame_once(waiter); } self.poll_resumed(waiter, Self::poll_read_frame_once) } - fn poll_read_frame_once(&mut self, waiter: &kio::Waiter) -> Poll>> { + /// Read the next frame with its relay metadata, for wire egress. + pub(crate) fn poll_read_object(&mut self, waiter: &kio::Waiter) -> Poll>> { + // Every frame is served through the cursor's prefetch, which holds its fields. + let frame = ready!(self.poll_read_frame(waiter))?; + Poll::Ready(Ok(frame.map(|frame| frame::Object { + frame, + fields: self.cursor.prefetch.take_fields(), + }))) + } + + fn poll_read_frame_once(&mut self, waiter: &kio::Waiter) -> Poll>> { if self.ended { return Poll::Ready(Ok(None)); } @@ -1843,7 +1888,7 @@ impl Consumer { { self.cursor.refresh_if_stale(); self.cursor.index += 1; - return Ok(Some(frame.frame)); + return Ok(Some(frame)); } kio::wait(|waiter| self.poll_read_frame(waiter)).await } @@ -2023,10 +2068,10 @@ impl Cursor { size: frame.payload.len() as u64, timestamp: frame.timestamp, }; - let source = frame::Source::Complete(frame.frame.payload); + let source = frame::Source::Complete(frame.payload); let header = frame::Header { info, - fields: frame.fields, + fields: self.prefetch.take_fields(), }; let frame = frame::Consumer::new(self.state.clone(), header, source); return Poll::Ready(Ok(Some(match expiry { @@ -2051,7 +2096,7 @@ impl Cursor { }))) } - fn poll_read_frame(&mut self, waiter: &kio::Waiter, stats: &stats::Meter) -> Poll>> { + fn poll_read_frame(&mut self, waiter: &kio::Waiter, stats: &stats::Meter) -> Poll>> { if self.capped() { return Poll::Ready(Ok(None)); } @@ -2079,7 +2124,7 @@ impl Cursor { // out-of-bounds start. `fill` always resets the batch, so an empty range // leaves `len == 0` and the terminal checks below resolve abort/fin/pending. let local = (index - state.offset).min(state.frames.len()); - prefetch.fill(state.frames.range(local..).take(budget).cloned()); + prefetch.fill(state, local, budget); if prefetch.len > 0 { // One stamp covers the whole batch: frames popped from the prefetch // don't re-stamp until the next refill, which `CAP` bounds. @@ -2174,6 +2219,75 @@ mod test { #[cfg(feature = "async")] use futures::FutureExt; + /// Fields sit beside the frames only from the first fielded one, so every read path + /// must line them up by index across plain frames and prefetch batches. + #[test] + fn object_fields_follow_their_frames_through_every_read() { + let pool = cache::Pool::unbounded(); + let cache = cache::Track::new(pool.clone(), kio::Weak::new()); + let mut group = Producer::new(Info::from(0u64), track::Info::default(), cache); + group.write_frame(None, Bytes::from_static(b"p")).unwrap(); + let plain = pool.used(); + // Ten fielded frames span two prefetch batches, then plain frames follow. + for id in 1..=10u64 { + let header = frame::Header { + info: frame::Info { + size: 1, + timestamp: None, + }, + fields: Some(Arc::new(super::super::object::Fields { + id: id * 10, + properties: Default::default(), + })), + }; + let mut frame = group.create_frame_owned(header, &frame::Budget::default()).unwrap(); + frame.write(Bytes::from_static(b"f")).unwrap(); + frame.finish().unwrap(); + } + assert!(pool.used() > plain + 10, "the metadata table is charged"); + for _ in 0..Prefetch::CAP { + group.write_frame(None, Bytes::from_static(b"p")).unwrap(); + } + group.finish().unwrap(); + let expected = |index: u64| (1..=10).contains(&index).then_some(index * 10); + + let mut objects = group.consume(); + let waiter = kio::Waiter::noop(); + for index in 0..(11 + Prefetch::CAP as u64) { + let Poll::Ready(Ok(Some(object))) = objects.poll_read_object(&waiter) else { + panic!("frame {index} is ready"); + }; + assert_eq!(object.fields.map(|fields| fields.id), expected(index), "frame {index}"); + } + + // A public read leaves the batch's fields for a later streamed read to find. + let mut mixed = group.consume(); + mixed.read_frame().now_or_never().unwrap().unwrap().unwrap(); + for index in 1..Prefetch::CAP as u64 { + let frame = mixed.next_frame().now_or_never().unwrap().unwrap().unwrap(); + assert_eq!( + frame.fields.as_ref().map(|fields| fields.id), + expected(index), + "frame {index}" + ); + } + // Public reads never take a batch's fields, so a plain batch after them must not + // hand the stale ones to a wire read. + for _ in 0..Prefetch::CAP { + mixed.read_frame().now_or_never().unwrap().unwrap().unwrap(); + } + for index in (2 * Prefetch::CAP as u64)..(11 + Prefetch::CAP as u64) { + let Poll::Ready(Ok(Some(object))) = mixed.poll_read_object(&waiter) else { + panic!("frame {index} is ready"); + }; + assert_eq!(object.fields.map(|fields| fields.id), None, "frame {index}"); + } + + drop((objects, mixed)); + group.abort(Error::Cancel).unwrap(); + assert_eq!(pool.used(), 0); + } + #[test] fn terminal_metadata_stays_charged_after_frames_are_released() { let pool = cache::Pool::unbounded(); @@ -2251,7 +2365,7 @@ mod test { assert!(capacity > FRAME_SLOTS); assert_eq!( pool.used() - before, - ((capacity - FRAME_SLOTS) * size_of::()) as u64 + ((capacity - FRAME_SLOTS) * size_of::()) as u64 ); let consumer = group.consume(); group.abort(Error::Cancel).unwrap(); @@ -2263,14 +2377,11 @@ mod test { /// would undercharge every cached group without touching a line of this crate. #[test] fn one_frame_fits_the_charged_slots() { - let mut frames: VecDeque = VecDeque::new(); - frames.push_back( - Frame { - timestamp: Some(Timestamp::ZERO), - payload: Bytes::new(), - } - .into(), - ); + let mut frames: VecDeque = VecDeque::new(); + frames.push_back(Frame { + timestamp: Some(Timestamp::ZERO), + payload: Bytes::new(), + }); let capacity = frames.capacity(); assert!( capacity <= FRAME_SLOTS, From 53f867f9a362b7c75284e72632dd0662fcb05b1a Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 14 Oct 2026 10:38:35 -0700 Subject: [PATCH 04/12] perf(net): box a group's IETF relay state Co-Authored-By: Claude Opus 5.5 --- rs/moq-net/src/model/frame.rs | 2 +- rs/moq-net/src/model/group.rs | 118 ++++++++++++++++++++++++---------- 2 files changed, 84 insertions(+), 36 deletions(-) diff --git a/rs/moq-net/src/model/frame.rs b/rs/moq-net/src/model/frame.rs index 2b7b2359068..8e5055842e4 100644 --- a/rs/moq-net/src/model/frame.rs +++ b/rs/moq-net/src/model/frame.rs @@ -780,7 +780,7 @@ impl Consumer { if !recover.wants(failed.as_ref(), waiter) { return res; } - if self.state.read().wire.as_ref().is_some_and(|wire| wire.sparse) { + if self.state.read().sparse() { return Poll::Ready(Err(Error::Unsupported)); } let mut replacement = match recover.poll(*index, failed.as_ref(), waiter) { diff --git a/rs/moq-net/src/model/group.rs b/rs/moq-net/src/model/group.rs index 40850b3d8d9..89c8674aca1 100644 --- a/rs/moq-net/src/model/group.rs +++ b/rs/moq-net/src/model/group.rs @@ -138,13 +138,8 @@ pub(crate) struct GroupState { // Completed frames, each a contiguous payload. `offset` is the first frame this // handle holds, raised by [`Producer::start_at`]. pub(crate) frames: VecDeque, - // Relay metadata for `frames`, at the same local index. Frames past its end carry - // none, so a group that never sees a field never allocates it or pays to skip it. - objects: VecDeque>>, - // Known before frames arrive; the terminal object is learned at the stream tail. - pub(crate) wire: Option, - pub(crate) end_object: Option>, - metadata_bytes: u64, + // IETF relay state, boxed so a group that never carries any pays one pointer. + relay: Option>, // The single in-flight frame, if one is open. pub(crate) partial: Option, @@ -190,6 +185,19 @@ pub(crate) struct GroupState { pub(crate) abort: Option, } +/// What an IETF relay keeps beside a group's frames. +#[derive(Default)] +struct Relay { + // Known before frames arrive. + wire: Option, + // The terminal object, learned at the stream tail. + end_object: Option>, + // Fields for `frames`, at the same local index. Frames past its end carry none. + objects: VecDeque>>, + // Heap charged for this box, its fields, and the terminal object. + bytes: u64, +} + /// The IETF header state that cannot be inferred from a plain frame list. #[derive(Clone, Debug)] pub(crate) struct Wire { @@ -364,11 +372,37 @@ impl GroupState { self.next_index.saturating_sub(self.offset).saturating_add(extra_frames) > MAX_GROUP_FRAMES || self .cache - .saturating_add(self.metadata_bytes) + .saturating_add(self.relay.as_ref().map_or(0, |relay| relay.bytes)) .saturating_add(extra_bytes) > MAX_CACHE_BYTES } + /// The relay state, charging its box the first time a group needs it. + fn relay(&mut self) -> &mut Relay { + let charge = &mut self.charge; + self.relay.get_or_insert_with(|| { + let bytes = size_of::() as u64; + charge.add(bytes); + Box::new(Relay { + bytes, + ..Default::default() + }) + }) + } + + pub(crate) fn wire(&self) -> Option<&Wire> { + self.relay.as_ref()?.wire.as_ref() + } + + fn wire_mut(&mut self) -> Option<&mut Wire> { + self.relay.as_mut()?.wire.as_mut() + } + + /// Whether Object IDs left the frame ordinals, so ordinal recovery cannot stitch. + pub(crate) fn sparse(&self) -> bool { + self.wire().is_some_and(|wire| wire.sparse) + } + /// Reserve the next completed frame, returning growth not covered by the fixed overhead. fn reserve_frame(&mut self) -> u64 { let before = self.frames.capacity().saturating_sub(FRAME_SLOTS); @@ -379,15 +413,22 @@ impl GroupState { /// Reserve a metadata slot for the next completed frame, returning the growth. fn reserve_fields(&mut self) -> u64 { - let before = self.objects.capacity(); - self.objects.resize(self.frames.len(), None); - self.objects.reserve(1); - ((self.objects.capacity() - before) * size_of::>>()) as u64 + let len = self.frames.len(); + let objects = &mut self.relay().objects; + let before = objects.capacity(); + objects.resize(len, None); + objects.reserve(1); + ((objects.capacity() - before) * size_of::>>()) as u64 + } + + /// How many completed frames, from the first, the fields table covers. + fn objects(&self) -> usize { + self.relay.as_ref().map_or(0, |relay| relay.objects.len()) } /// The relay metadata of the completed frame at `local`, if it carried any. fn fields(&self, local: usize) -> Option> { - self.objects.get(local).cloned().flatten() + self.relay.as_ref()?.objects.get(local).cloned().flatten() } /// Charge the in-flight frame's bytes written since its last charge, as a write access. @@ -413,12 +454,15 @@ impl GroupState { /// Drop the cached frames (and any in-flight tail) and release their pool charge. fn release(&mut self) { self.frames = VecDeque::new(); - self.objects = VecDeque::new(); self.partial = None; - self.metadata_bytes = self.end_object.as_ref().map_or(0, |fields| fields.heap()); self.cache = 0; self.charge.clear(); - self.charge.add(self.metadata_bytes); + // The header state and terminal object outlive the frames. + if let Some(relay) = &mut self.relay { + relay.objects = VecDeque::new(); + relay.bytes = size_of::() as u64 + relay.end_object.as_ref().map_or(0, |fields| fields.heap()); + self.charge.add(relay.bytes); + } } } @@ -539,11 +583,11 @@ impl std::ops::Deref for Producer { impl Producer { pub(crate) fn wire(&self) -> Option { - self.state.read().wire.clone() + self.state.read().wire().cloned() } pub(crate) fn set_wire(&self, wire: Wire) -> Result<()> { - modify(&self.state)?.wire = Some(wire); + modify(&self.state)?.relay().wire = Some(wire); Ok(()) } @@ -553,7 +597,7 @@ impl Producer { } let mut state = modify(&self.state)?; let offset = state.offset as u64; - if let Some(wire) = &mut state.wire { + if let Some(wire) = state.wire_mut() { let missing = match wire.last { None => id.checked_sub(offset).ok_or(Error::MalformedTrack)?, Some(prior) => id @@ -571,7 +615,7 @@ impl Producer { pub(crate) fn set_end_object(&self, fields: super::object::Fields) -> Result<()> { let mut state = modify(&self.state)?; - if state.fin.is_some() || state.end_object.is_some() { + if state.fin.is_some() || state.relay.as_ref().is_some_and(|relay| relay.end_object.is_some()) { return Err(Error::MalformedTrack); } let bytes = fields.heap(); @@ -579,11 +623,12 @@ impl Producer { return Err(Error::GroupTooLarge); } state.charge.add(bytes); - state.metadata_bytes += bytes; - if let Some(wire) = &mut state.wire { + let relay = state.relay(); + relay.bytes += bytes; + if let Some(wire) = &mut relay.wire { wire.last = Some(fields.id); } - state.end_object = Some(Arc::new(fields)); + relay.end_object = Some(Arc::new(fields)); Ok(()) } @@ -860,14 +905,16 @@ impl Producer { buf: buf.clone(), charged: 0, }); - state.metadata_bytes += metadata_bytes; - state.charge.add(metadata_bytes); + if metadata_bytes > 0 { + state.relay().bytes += metadata_bytes; + state.charge.add(metadata_bytes); + } let object_id = state .partial .as_ref() .and_then(|partial| partial.fields.as_ref()) .map_or(state.next_index as u64, |fields| fields.id); - if let Some(wire) = &mut state.wire { + if let Some(wire) = state.wire_mut() { wire.last = Some(object_id); wire.sparse |= object_id != (next_index - 1) as u64; } @@ -921,8 +968,9 @@ impl Producer { if let Some(fields) = state.partial.take().and_then(|partial| partial.fields) { // The slot was reserved and charged by `charge_partial` above. let len = state.frames.len(); - state.objects.resize(len, None); - state.objects.push_back(Some(fields)); + let objects = &mut state.relay().objects; + objects.resize(len, None); + objects.push_back(Some(fields)); } state.frames.push_back(frame); state.committed = state.next_index; @@ -1067,7 +1115,7 @@ impl Producer { /// tail, and the count outlives the released cache. pub(crate) fn resume_frame(&self) -> Option { let state = self.state.read(); - if state.fin.is_some() || state.wire.as_ref().is_some_and(|wire| wire.sparse) { + if state.fin.is_some() || state.sparse() { return None; } (state.committed > state.offset).then_some(state.committed) @@ -1376,7 +1424,7 @@ impl Prefetch { self.len += 1; } // Plain frames never reach the metadata table, so they skip this entirely. - if local < state.objects.len() { + if local < state.objects() { let fields = self.fields.get_or_insert_default(); for (i, slot) in fields.iter_mut().enumerate() { *slot = if i < self.len { state.fields(local + i) } else { None }; @@ -1530,18 +1578,18 @@ impl std::ops::Deref for Consumer { impl Consumer { pub(crate) fn last_object(&self) -> Option { let state = self.cursor.state.read(); - match &state.wire { + match state.wire() { Some(wire) => wire.last, None => (state.next_index as u64).checked_sub(1), } } pub(crate) fn wire(&self) -> Option { - self.cursor.state.read().wire.clone() + self.cursor.state.read().wire().cloned() } pub(crate) fn end_object(&self) -> Option> { - self.cursor.state.read().end_object.clone() + self.cursor.state.read().relay.as_ref()?.end_object.clone() } /// Fill a private batch without losing metadata through the public frame view. @@ -1619,7 +1667,7 @@ impl Consumer { return res; } // An ordinal FETCH would request different content after an Object ID gap. - if self.cursor.state.read().wire.as_ref().is_some_and(|wire| wire.sparse) { + if self.cursor.state.read().sparse() { return Poll::Ready(Err(Error::Unsupported)); } let replacement = match recover.poll(self.cursor.index as u64, failed.as_ref(), waiter) { @@ -2298,7 +2346,7 @@ mod test { id: 4, properties: Default::default(), }; - let charged = fields.heap(); + let charged = size_of::() as u64 + fields.heap(); group.set_end_object(fields.clone()).unwrap(); assert!(matches!(group.set_end_object(fields), Err(Error::MalformedTrack))); group.finish().unwrap(); From 889d32f22181710d25338f131cca7678e7039706 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 14 Oct 2026 10:59:16 -0700 Subject: [PATCH 05/12] bench(net): keep the relay sweep off the read sweep's instantiation Co-Authored-By: Claude Opus 5.5 --- rs/moq-net/benches/group.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/rs/moq-net/benches/group.rs b/rs/moq-net/benches/group.rs index b8ca1fb0c1b..1446fbcd405 100644 --- a/rs/moq-net/benches/group.rs +++ b/rs/moq-net/benches/group.rs @@ -334,7 +334,9 @@ fn bench_relay_frames(c: &mut Criterion) { b.iter_batched( || (0..subscribers).map(|_| context.2.consume()).collect::>(), |mut readers| { - let mut buffer = frame::Buffer::<8>::new(); + // A capacity `group_read_frames` doesn't sweep: sharing its readers' + // instantiation moves them out of line and shifts that sweep. + let mut buffer = frame::Buffer::<16>::new(); for reader in &mut readers { drain_batched(reader, &mut buffer); } From 5d309b16cdcce4dac472c5ab193284ce858c1918 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 14 Oct 2026 12:52:02 -0700 Subject: [PATCH 06/12] fix(net): convert relayed timestamps and keep datagrams best-effort Relayed Timestamp properties leave in the units the egress declares, and an immutable one that would change units is refused. A relayed datagram the downstream draft cannot carry is dropped instead of ending the subscription, and an omitted datagram priority resolves to the track default. Co-Authored-By: Claude Opus 5.5 --- doc/concept/standard.md | 5 +- quest/m1/ietf-extension/README.md | 2 +- rs/moq-net/src/ietf/object.rs | 95 +++++++++++++++++++++++++++---- rs/moq-net/src/ietf/publisher.rs | 52 +++++++++++------ rs/moq-net/src/ietf/subscriber.rs | 46 ++++++++++++++- rs/moq-net/src/model/object.rs | 7 +++ 6 files changed, 172 insertions(+), 35 deletions(-) diff --git a/doc/concept/standard.md b/doc/concept/standard.md index fb09123a8de..079d1da2e57 100644 --- a/doc/concept/standard.md +++ b/doc/concept/standard.md @@ -31,7 +31,7 @@ subset drops. What a peer actually observes against this implementation: - **Strict SETUP and parameters.** Repeated unknown `SETUP` options, GREASE included, are accepted if well-formed; a repeated known option is rejected. On draft 17 and later, a `GROUP_ORDER` other than Ascending (1) or Descending (2) closes the session. Message parameters follow the negotiated draft's lists: drafts 14 and 15 ignore an unknown or misplaced parameter, draft 16 ignores one defined only for another message but closes the session on an unknown one, and draft 17 and later close it on both. - **One `SETUP` credential**, forwarded to the [auth server](/bin/relay/auth#the-contract) unverified. A second `SETUP` token closes the session. We advertise no token cache, so an alias reference in `SETUP` closes it with `PROTOCOL_VIOLATION`, one on a request with `UNKNOWN_AUTH_TOKEN_ALIAS`, and a registration on a request with `AUTH_TOKEN_CACHE_OVERFLOW`. A token attached to an individual request is otherwise decoded and grants nothing, and a request repeating one is refused with `MALFORMED_AUTH_TOKEN`. `SUBSCRIBE_TRACKS` is refused unread, so its token is not decoded. On draft 17 and later, the negotiated [MoQ Auth extension](/concept/moq-lite#authorization) accepts additional session tokens in band. - **Refused, not fatal.** A legal request this stack does not serve is rejected on its own and the session stays up: `FORWARD=0`, range filters, `SUBSCRIBE_TRACKS`, and the fetch forms above. On draft 19 and later, and in Rust on drafts 14 through 16, a subscription update may change only priority; any other update ends that subscription. -- **Objects and datagrams.** Rust relays preserve Object IDs, gaps, object properties, FIRST\_OBJECT, and END\_OF\_GROUP status objects between moq-transport peers for subgroup 0. Datagrams retain their IDs, properties, priority, and status. Plain Rust readers and moq-lite peers see consecutive frames and normal datagram payloads; JavaScript supports only normal datagrams at Object ID 0. A malformed datagram closes the session. Sparse groups cannot resume across route changes using frame ordinals; recovery refuses the group instead of requesting the wrong objects. +- **Objects and datagrams.** Rust relays preserve Object IDs, gaps, object properties, FIRST\_OBJECT, and END\_OF\_GROUP status objects between moq-transport peers for subgroup 0. Datagrams retain their IDs, properties, priority, and status. Plain Rust readers and moq-lite peers see consecutive frames and normal datagram payloads; JavaScript supports only normal datagrams at Object ID 0. A malformed datagram closes the session; a relayed one the downstream draft cannot carry is dropped. A relayed Timestamp leaves in the units the downstream response declares. Sparse groups cannot resume across route changes using frame ordinals; recovery refuses the group instead of requesting the wrong objects. - **Priority.** Higher is served first. The IETF default of 128 is this stack's 127, and a track that never sets one is 127. - **Size.** An object extension block larger than 64 KiB ends that subgroup stream. The session stays up. This cap is ours, not the draft's. @@ -67,7 +67,8 @@ product's model, not bugs, and the relay does not change them. it. Rust moq-transport relay paths do so; the public API, plain moq-lite, and JavaScript [leave other per-object metadata out](/concept/moq-lite#what-moq-lite-leaves-out). Immutable property blocks retain their bytes. Forwarding is refused when the - negotiated drafts use incompatible key or integer encodings. + negotiated drafts use incompatible key or integer encodings, or when the block + holds a Timestamp in units the response does not declare. - **`SUBSCRIBE_OK` before an old source answers.** moq-lite 01 through 04 have no track stream, so the relay cannot learn from that source whether the track exists before answering. A moq-transport subscriber gets `SUBSCRIBE_OK` diff --git a/quest/m1/ietf-extension/README.md b/quest/m1/ietf-extension/README.md index a4350a56e2a..9157c91101a 100644 --- a/quest/m1/ietf-extension/README.md +++ b/quest/m1/ietf-extension/README.md @@ -31,7 +31,7 @@ That decline's cache-fragmentation concern does not apply here, because private Object IDs distinguish gaps from frame ordinals, and a FETCH it cannot answer unambiguously is refused. -- **Carried:** subgroups with their publisher priority, object ID gaps as a +- **Carried:** subgroups with their publisher priority, object ID gaps as crate-private absolute Object IDs, object properties as a decoded KVP list, END_OF_GROUP status objects at their object ID, datagrams past object 0, and unknown track properties. diff --git a/rs/moq-net/src/ietf/object.rs b/rs/moq-net/src/ietf/object.rs index a1067097139..28eba0d738e 100644 --- a/rs/moq-net/src/ietf/object.rs +++ b/rs/moq-net/src/ietf/object.rs @@ -26,6 +26,7 @@ struct List { properties: Properties, time: Option, gap: Option, + sealed_time: bool, } fn list(data: &Bytes, version: Version, immutable: bool) -> Result { @@ -35,6 +36,7 @@ fn list(data: &Bytes, version: Version, immutable: bool) -> Result properties: Properties::default(), time: None, gap: None, + sealed_time: false, }; let mut previous = 0u64; while !decoder.is_empty() { @@ -71,6 +73,7 @@ fn list(data: &Bytes, version: Version, immutable: bool) -> Result return Err(Error::MalformedTrack); } let nested = list(&bytes, version, true).map_err(|_| Error::MalformedTrack)?; + result.sealed_time = nested.time.is_some(); result.time = result.time.or(nested.time); if let Some(gap) = nested.gap && result.gap.replace(gap).is_some() @@ -86,7 +89,9 @@ fn list(data: &Bytes, version: Version, immutable: bool) -> Result } pub(super) fn read(data: &Bytes, version: Version, timescale: Option) -> Result { - let parsed = list(data, version, false)?; + let mut parsed = list(data, version, false)?; + parsed.properties.timescale = timescale.filter(|_| parsed.time.is_some()); + parsed.properties.sealed_time = parsed.sealed_time; let timestamp = match (parsed.time, timescale) { (Some(value), Some(timescale)) => Some(Timestamp::new(value, timescale).map_err(|_| Error::MalformedTrack)?), _ => None, @@ -98,18 +103,62 @@ pub(super) fn read(data: &Bytes, version: Version, timescale: Option) }) } -pub(super) fn write(properties: &Properties, version: Version) -> Result, Error> { +/// Serialize relayed properties for `version`, with Timestamps in the `timescale` the +/// egress declares, if any. +pub(super) fn write(properties: &Properties, version: Version, timescale: Option) -> Result, Error> { if properties .immutable .is_some_and(|original| original != encoding(version)) { return Err(Error::Unsupported); } - let timestamp = properties.timestamp.map(|value| Property { - id: 0x10, - value: Value::Int(value), - }); - let mut sorted: Vec<_> = properties.values.iter().chain(timestamp.as_ref()).collect(); + // Without both units there is nothing to convert: an untimed track forwards them as is. + let convert = match (properties.timescale, timescale) { + (Some(from), Some(to)) if from != to => Some((from, to)), + _ => None, + }; + let time = |value: u64| -> Result { + Ok(Value::Int(match convert { + Some((from, to)) => Timestamp::new(value, from) + .and_then(|timestamp| timestamp.convert(to)) + .map_err(|_| Error::Unsupported)? + .value(), + None => value, + })) + }; + // Rewriting an immutable Timestamp would break it; forwarding it would change its meaning. + if properties.sealed_time && convert.is_some() { + return Err(Error::Unsupported); + } + let timestamp = properties + .timestamp + .map(|value| { + Ok::<_, Error>(Property { + id: 0x10, + value: time(value)?, + }) + }) + .transpose()?; + // The draft-03 Timestamp key carries the same units. + let legacy = properties + .values + .iter() + .filter(|property| property.id == 0x06) + .map(|property| match property.value { + Value::Int(value) => Ok(Property { + id: 0x06, + value: time(value)?, + }), + _ => Err(Error::MalformedTrack), + }) + .collect::, _>>()?; + let mut sorted: Vec<_> = properties + .values + .iter() + .filter(|property| property.id != 0x06) + .chain(&legacy) + .chain(timestamp.as_ref()) + .collect(); // Delta keys require ascending order. Equal keys retain the publisher's order. sorted.sort_by_key(|property| property.id); let mut out = Vec::new(); @@ -176,11 +225,35 @@ mod tests { let decoded = read(&bytes, Version::Draft22, Some(Timescale::MILLI)).unwrap(); assert_eq!(decoded.timestamp, Some(Timestamp::from_millis(42).unwrap())); assert_eq!(decoded.properties.timestamp, None); - assert_eq!(write(&decoded.properties, Version::Draft18).unwrap(), bytes); + let milli = Some(Timescale::MILLI); + assert_eq!(write(&decoded.properties, Version::Draft18, milli).unwrap(), bytes); assert!(matches!( - write(&decoded.properties, Version::Draft17), + write(&decoded.properties, Version::Draft17, milli), Err(Error::Unsupported) )); + // Its units cannot follow the egress's without rewriting the block. + assert!(matches!( + write(&decoded.properties, Version::Draft18, Some(Timescale::MICRO)), + Err(Error::Unsupported) + )); + } + + /// The cache keeps a timed track in its own units, so a relayed Timestamp must leave + /// in the units the egress declares, not the ones it arrived in. + #[test] + fn mutable_timestamps_follow_the_egress_units() { + for key in [0x10, 0x06] { + let bytes = Bytes::copy_from_slice(&[key, 42, 0x20, 7]); + let decoded = read(&bytes, Version::Draft22, Some(Timescale::MILLI)).unwrap(); + let written = write(&decoded.properties, Version::Draft22, Some(Timescale::MICRO)).unwrap(); + let again = read(&Bytes::from(written), Version::Draft22, Some(Timescale::MICRO)).unwrap(); + let micros = Timestamp::new(42_000, Timescale::MICRO).unwrap(); + assert_eq!(again.timestamp, Some(micros), "key {key:#x}"); + assert_eq!(again.properties.values.last().unwrap().value, Value::Int(7)); + + // An egress that declares no units forwards the value untouched. + assert_eq!(write(&decoded.properties, Version::Draft22, None).unwrap(), bytes); + } } #[test] @@ -191,10 +264,10 @@ mod tests { // Draft-18 permits this nonminimal seven-byte value; draft-17 cannot read it. let bytes = Bytes::from_static(&[0x0b, 8, 2, 0xfc, 0, 0, 0, 0, 0, 1]); let decoded = read(&bytes, Version::Draft18, None).unwrap(); - assert_eq!(write(&decoded.properties, Version::Draft22).unwrap(), bytes); + assert_eq!(write(&decoded.properties, Version::Draft22, None).unwrap(), bytes); assert!(read(&bytes, Version::Draft17, None).is_err()); assert!(matches!( - write(&decoded.properties, Version::Draft17), + write(&decoded.properties, Version::Draft17, None), Err(Error::Unsupported) )); } diff --git a/rs/moq-net/src/ietf/publisher.rs b/rs/moq-net/src/ietf/publisher.rs index 7498dd970bd..41884a86be0 100644 --- a/rs/moq-net/src/ietf/publisher.rs +++ b/rs/moq-net/src/ietf/publisher.rs @@ -1475,7 +1475,7 @@ where // timestamp only on drafts that can declare its units. let timescale = timescale.filter(|_| ietf::Properties::sends_timescale(version)); let properties = if let Some(fields) = fields { - let properties = super::object::write(&fields.properties, version)?; + let properties = super::object::write(&fields.properties, version, timescale)?; (!properties.is_empty()).then_some(properties) } else { match timescale { @@ -1770,9 +1770,15 @@ where Some(Err(err)) => return self.reject_fetch(stream, msg.request_id, &err, &err.to_string()).await, None => return Ok(()), }; + // FETCH keeps each object's Timestamp, in the units FETCH_OK declares wherever the + // draft can declare them. Opting out of the properties strips nothing from the objects. + let timescale = group + .info + .timescale + .filter(|_| ietf::Properties::sends_timescale(self.version)); for frame in &group.frames { if let Some(fields) = &frame.fields - && let Err(err) = super::object::write(&fields.properties, self.version) + && let Err(err) = super::object::write(&fields.properties, self.version, timescale) { return self.reject_fetch(stream, msg.request_id, &err, &err.to_string()).await; } @@ -1809,13 +1815,6 @@ where .await; } - // FETCH keeps each object's Timestamp, in the units FETCH_OK declares wherever the - // draft can declare them. Opting out of the properties strips nothing from the objects. - let timescale = group - .info - .timescale - .filter(|_| ietf::Properties::sends_timescale(self.version)); - let (end_location, end_of_track) = if joined { // The subscription starts at the saved Largest Object, so the prefix of that // group has to be here in full or the two leave a gap. @@ -3291,18 +3290,29 @@ impl TrackServe { } let (object_id, priority, end_of_group, status, properties) = match &datagram.wire { Some(wire) => { + // A datagram this draft cannot carry is dropped like an oversized one: it + // is best-effort, and the track's groups do not depend on it. if wire.status == Some(1) && !matches!(self.version, Version::Draft14 | Version::Draft15) { - return Err(Error::Unsupported); + tracing::debug!(sequence, "dropping a datagram status this draft cannot carry"); + continue; } - let properties = super::object::write(&wire.properties, self.version)?; + let properties = match super::object::write(&wire.properties, self.version, self.timescale) { + Ok(properties) => properties, + Err(err) => { + tracing::debug!(%err, sequence, "dropping a datagram whose properties cannot be forwarded"); + continue; + } + }; let object = if self.version == Version::Draft14 && wire.status.is_some() { Some(wire.object_id.unwrap_or(0)) } else { wire.object_id }; - let priority = wire.priority.or_else(|| { - (self.version == Version::Draft14).then(|| super::priority::to_wire(self.track.info().priority)) - }); + // An omitted priority inherited the source track's default, which this + // response may not carry (or carry the same), so resolve it here. + let priority = wire + .priority + .or_else(|| Some(super::priority::to_wire(self.track.info().priority))); ( object, priority, @@ -3342,8 +3352,14 @@ impl TrackServe { Some(status) => ietf::DatagramBody::Status(status), None => ietf::DatagramBody::Payload(datagram.datagram.payload), }, - } - .encode_bytes(self.version)?; + }; + let body = match body.encode_bytes(self.version) { + Ok(body) => body, + Err(err) => { + tracing::debug!(%err, sequence, "dropping a datagram this draft cannot encode"); + continue; + } + }; let max = self.session.max_datagram_size(); if body.len() > max { tracing::debug!( @@ -3812,7 +3828,7 @@ impl GroupServe { }, ObjectEncoding { extensions: self.msg.flags.has_extensions, - timescale: None, + timescale: self.timescale, version: self.version, }, )?; @@ -3932,7 +3948,7 @@ fn buffer_object( writer.buffer_varint(object.delta)?; if encoding.extensions { let ext = match object.fields { - Some(fields) => super::object::write(&fields.properties, encoding.version)?, + Some(fields) => super::object::write(&fields.properties, encoding.version, encoding.timescale)?, None => { let mut ext = Vec::new(); if let Some(timescale) = encoding.timescale { diff --git a/rs/moq-net/src/ietf/subscriber.rs b/rs/moq-net/src/ietf/subscriber.rs index 49d38217c35..978bb24b4c2 100644 --- a/rs/moq-net/src/ietf/subscriber.rs +++ b/rs/moq-net/src/ietf/subscriber.rs @@ -8499,12 +8499,26 @@ mod stitch_tests { ingress: Version, egress: Version, fetch: Option<(u64, u64)>, + ) -> (Vec, crate::lite::test_transport::Log) { + relay_passthrough_timed(script, ingress, egress, fetch, None).await + } + + /// Relay `script` through a track the ingress times in `timescale`, which the cache + /// normalizes to microseconds like [`accepted_info`]. + async fn relay_passthrough_timed( + script: Vec, + ingress: Version, + egress: Version, + fetch: Option<(u64, u64)>, + timescale: Option, ) -> (Vec, crate::lite::test_transport::Log) { use crate::lite::test_transport::Sent; let origin = crate::origin::Config::new(crate::Hop::new(2).unwrap()).produce(); let broadcast = origin.publish("room", crate::origin::Route::default()).unwrap(); - let track = broadcast.create_track("video", None).unwrap(); - let mut h = Harness::with_timescale(Fill::Done, vec![script], None); + let track = broadcast + .create_track("video", accepted_info(timescale, None, None)) + .unwrap(); + let mut h = Harness::with_timescale(Fill::Done, vec![script], timescale); h.subscriber.version = ingress; h.subscriber.state.lock().subscribes.get_mut(&REQUEST).unwrap().producer = Some(track.clone()); let mut stream = h.stream().await.with_version(ingress); @@ -8652,6 +8666,28 @@ mod stitch_tests { } } + /// A relayed Timestamp leaves in the units the egress declares: the cache holds the + /// track in microseconds, whatever the publisher used. + #[moq_net_sim::test] + async fn passthrough_converts_relayed_timestamps_to_the_egress_units() { + let version = Version::Draft22; + // Timestamp 42 (ms), then an unknown property that must survive beside it. + let properties = [0x10, 42, 0x70, 7]; + let script = object_stream(version, true, &[(0, &properties, None)]); + let (bytes, log) = relay_passthrough_timed(script, version, version, None, Some(Timescale::MILLI)).await; + assert!(log.resets().is_empty()); + let mut decoder = Decoder::new(&bytes, version.into()); + ietf::GroupHeader::decode(&mut decoder, version).unwrap(); + assert_eq!(decoder.varint().unwrap(), 0); + let relayed = bytes::Bytes::copy_from_slice(decoder.bytes().unwrap()); + let decoded = super::super::object::read(&relayed, version, Some(Timescale::MICRO)).unwrap(); + assert_eq!( + decoded.timestamp, + Some(Timestamp::new(42_000, Timescale::MICRO).unwrap()) + ); + assert_eq!(decoded.properties.values.len(), 1); + } + fn object_stream(version: Version, first: bool, objects: &[(u64, &[u8], Option)]) -> Vec { let mut bytes = Vec::new(); ietf::GroupHeader { @@ -8921,7 +8957,9 @@ mod stitch_tests { track_alias: ALIAS, group_id: SEQUENCE, object_id: Some(id), - publisher_priority: Some(200), + // An omitted priority inherits the track default, which the + // downstream response need not carry. + publisher_priority: (id != 6).then_some(200), end_of_group: false, properties: (id != 8).then(|| properties.clone()), body: if id == 8 { @@ -8985,8 +9023,10 @@ mod stitch_tests { let sent = downstream.log.datagrams.lock().unwrap(); original.retain(|datagram| !ranged || datagram.object_id == Some(7)); assert_eq!(sent.len(), original.len()); + let inherited = super::super::priority::to_wire(track.publisher_priority()); for (bytes, mut expected) in sent.iter().zip(original) { expected.track_alias = 3; + expected.publisher_priority = expected.publisher_priority.or(Some(inherited)); let (actual, _) = ietf::ObjectDatagram::decode_slice(bytes, version).unwrap(); assert_eq!(actual, expected); } diff --git a/rs/moq-net/src/model/object.rs b/rs/moq-net/src/model/object.rs index 077e93b0bf5..e1999bafaa0 100644 --- a/rs/moq-net/src/model/object.rs +++ b/rs/moq-net/src/model/object.rs @@ -1,6 +1,8 @@ //! Private relay metadata; public readers see only the frame or datagram payload. use bytes::Bytes; +use crate::Timescale; + /// How the publisher serialized the keys inside immutable properties. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub(crate) enum Encoding { @@ -27,6 +29,11 @@ pub(crate) struct Properties { pub timestamp: Option, pub values: Vec, pub immutable: Option, + /// The units of the Timestamps above, when the track declared any. The cache keeps the + /// track in its own units, so egress converts mutable ones to the units it declares. + pub timescale: Option, + /// The immutable block holds a Timestamp, which keeps its ingress units. + pub sealed_time: bool, } impl Properties { From 51bb404b67d7580950dacf66574e0449fc0c555b Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 14 Oct 2026 13:45:41 -0700 Subject: [PATCH 07/12] fix(net): gate session-only relay helpers behind async Co-Authored-By: Claude Opus 5.5 --- rs/moq-net/src/model/frame.rs | 3 +++ rs/moq-net/src/model/group.rs | 12 ++++++++++++ rs/moq-net/src/model/object.rs | 1 + rs/moq-net/src/model/resume.rs | 1 + 4 files changed, 17 insertions(+) diff --git a/rs/moq-net/src/model/frame.rs b/rs/moq-net/src/model/frame.rs index 8e5055842e4..061052adb77 100644 --- a/rs/moq-net/src/model/frame.rs +++ b/rs/moq-net/src/model/frame.rs @@ -49,12 +49,14 @@ pub struct Frame { } /// A frame with the relay metadata its group keeps beside it, read only by wire egress. +#[cfg(feature = "async")] #[derive(Clone, Debug)] pub(crate) struct Object { pub frame: Frame, pub fields: Option>, } +#[cfg(feature = "async")] impl std::ops::Deref for Object { type Target = Frame; fn deref(&self) -> &Self::Target { @@ -710,6 +712,7 @@ pub struct Consumer { // The group's channel, used to park while a partial frame fills. state: kio::Consumer, info: Info, + #[cfg_attr(not(feature = "async"), allow(dead_code))] pub(crate) fields: Option>, source: Source, // Byte offset consumed so far. diff --git a/rs/moq-net/src/model/group.rs b/rs/moq-net/src/model/group.rs index 89c8674aca1..b78ff8f023e 100644 --- a/rs/moq-net/src/model/group.rs +++ b/rs/moq-net/src/model/group.rs @@ -199,6 +199,7 @@ struct Relay { } /// The IETF header state that cannot be inferred from a plain frame list. +#[cfg_attr(not(feature = "async"), allow(dead_code))] #[derive(Clone, Debug)] pub(crate) struct Wire { /// Object IDs differ from the public frame ordinals, so ordinal recovery cannot stitch. @@ -582,15 +583,18 @@ impl std::ops::Deref for Producer { } impl Producer { + #[cfg(feature = "async")] pub(crate) fn wire(&self) -> Option { self.state.read().wire().cloned() } + #[cfg(feature = "async")] pub(crate) fn set_wire(&self, wire: Wire) -> Result<()> { modify(&self.state)?.relay().wire = Some(wire); Ok(()) } + #[cfg(feature = "async")] pub(crate) fn mark_gap(&self, id: u64, gap: Option) -> Result<()> { if gap.is_some_and(|gap| gap > id) { return Err(Error::MalformedTrack); @@ -613,6 +617,7 @@ impl Producer { Ok(()) } + #[cfg(feature = "async")] pub(crate) fn set_end_object(&self, fields: super::object::Fields) -> Result<()> { let mut state = modify(&self.state)?; if state.fin.is_some() || state.relay.as_ref().is_some_and(|relay| relay.end_object.is_some()) { @@ -1576,6 +1581,7 @@ impl std::ops::Deref for Consumer { } impl Consumer { + #[cfg(feature = "async")] pub(crate) fn last_object(&self) -> Option { let state = self.cursor.state.read(); match state.wire() { @@ -1584,15 +1590,18 @@ impl Consumer { } } + #[cfg(feature = "async")] pub(crate) fn wire(&self) -> Option { self.cursor.state.read().wire().cloned() } + #[cfg(feature = "async")] pub(crate) fn end_object(&self) -> Option> { self.cursor.state.read().relay.as_ref()?.end_object.clone() } /// Fill a private batch without losing metadata through the public frame view. + #[cfg(feature = "async")] pub(crate) fn poll_read_objects( &mut self, waiter: &kio::Waiter, @@ -1900,6 +1909,7 @@ impl Consumer { } /// Read the next frame with its relay metadata, for wire egress. + #[cfg(feature = "async")] pub(crate) fn poll_read_object(&mut self, waiter: &kio::Waiter) -> Poll>> { // Every frame is served through the cursor's prefetch, which holds its fields. let frame = ready!(self.poll_read_frame(waiter))?; @@ -2270,6 +2280,7 @@ mod test { /// Fields sit beside the frames only from the first fielded one, so every read path /// must line them up by index across plain frames and prefetch batches. #[test] + #[cfg(feature = "async")] fn object_fields_follow_their_frames_through_every_read() { let pool = cache::Pool::unbounded(); let cache = cache::Track::new(pool.clone(), kio::Weak::new()); @@ -2337,6 +2348,7 @@ mod test { } #[test] + #[cfg(feature = "async")] fn terminal_metadata_stays_charged_after_frames_are_released() { let pool = cache::Pool::unbounded(); let cache = cache::Track::new(pool.clone(), kio::Weak::new()); diff --git a/rs/moq-net/src/model/object.rs b/rs/moq-net/src/model/object.rs index e1999bafaa0..4ddc0218c00 100644 --- a/rs/moq-net/src/model/object.rs +++ b/rs/moq-net/src/model/object.rs @@ -37,6 +37,7 @@ pub(crate) struct Properties { } impl Properties { + #[cfg(feature = "async")] pub fn is_plain(&self) -> bool { self.values.is_empty() } diff --git a/rs/moq-net/src/model/resume.rs b/rs/moq-net/src/model/resume.rs index 98830fc7e23..4e0985f884a 100644 --- a/rs/moq-net/src/model/resume.rs +++ b/rs/moq-net/src/model/resume.rs @@ -2061,6 +2061,7 @@ mod test { } #[test] + #[cfg(feature = "async")] fn sparse_object_ids_refuse_ordinal_recovery() { let routes = Producer::new(); let logical = logical(&routes); From b7cd012db8af34defe919cd54648a583572f998e Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 14 Oct 2026 15:33:03 -0700 Subject: [PATCH 08/12] fix(net): keep relayed Object IDs apart from frame ordinals A FETCH starting inside a cached group's leading gap skipped its objects, because the start Object ID was used as a frame ordinal. A relayed group is now read from its first frame and filtered by ID. A first object's Prior Object ID Gap may cover IDs below where a fetched copy starts, so it no longer makes that copy unfetchable. Route recovery spliced a different object's bytes when the replacement copy began past a gap. A replacement frame must now carry the same Object ID, and an adopted copy must be dense through the first frame it returns. Datagram replay suppression after a route change keys on Group and Object ID, so later objects of an already-seen group are still handed out. Co-Authored-By: Claude Opus 5.5 --- doc/concept/standard.md | 2 +- rs/moq-net/src/ietf/publisher.rs | 5 +- rs/moq-net/src/ietf/subscriber.rs | 2 +- rs/moq-net/src/model/frame.rs | 5 ++ rs/moq-net/src/model/group.rs | 45 +++++++++- rs/moq-net/src/model/resume.rs | 144 ++++++++++++++++++++++++++++-- 6 files changed, 193 insertions(+), 10 deletions(-) diff --git a/doc/concept/standard.md b/doc/concept/standard.md index 079d1da2e57..e11967a8f26 100644 --- a/doc/concept/standard.md +++ b/doc/concept/standard.md @@ -31,7 +31,7 @@ subset drops. What a peer actually observes against this implementation: - **Strict SETUP and parameters.** Repeated unknown `SETUP` options, GREASE included, are accepted if well-formed; a repeated known option is rejected. On draft 17 and later, a `GROUP_ORDER` other than Ascending (1) or Descending (2) closes the session. Message parameters follow the negotiated draft's lists: drafts 14 and 15 ignore an unknown or misplaced parameter, draft 16 ignores one defined only for another message but closes the session on an unknown one, and draft 17 and later close it on both. - **One `SETUP` credential**, forwarded to the [auth server](/bin/relay/auth#the-contract) unverified. A second `SETUP` token closes the session. We advertise no token cache, so an alias reference in `SETUP` closes it with `PROTOCOL_VIOLATION`, one on a request with `UNKNOWN_AUTH_TOKEN_ALIAS`, and a registration on a request with `AUTH_TOKEN_CACHE_OVERFLOW`. A token attached to an individual request is otherwise decoded and grants nothing, and a request repeating one is refused with `MALFORMED_AUTH_TOKEN`. `SUBSCRIBE_TRACKS` is refused unread, so its token is not decoded. On draft 17 and later, the negotiated [MoQ Auth extension](/concept/moq-lite#authorization) accepts additional session tokens in band. - **Refused, not fatal.** A legal request this stack does not serve is rejected on its own and the session stays up: `FORWARD=0`, range filters, `SUBSCRIBE_TRACKS`, and the fetch forms above. On draft 19 and later, and in Rust on drafts 14 through 16, a subscription update may change only priority; any other update ends that subscription. -- **Objects and datagrams.** Rust relays preserve Object IDs, gaps, object properties, FIRST\_OBJECT, and END\_OF\_GROUP status objects between moq-transport peers for subgroup 0. Datagrams retain their IDs, properties, priority, and status. Plain Rust readers and moq-lite peers see consecutive frames and normal datagram payloads; JavaScript supports only normal datagrams at Object ID 0. A malformed datagram closes the session; a relayed one the downstream draft cannot carry is dropped. A relayed Timestamp leaves in the units the downstream response declares. Sparse groups cannot resume across route changes using frame ordinals; recovery refuses the group instead of requesting the wrong objects. +- **Objects and datagrams.** Rust relays preserve Object IDs, gaps, object properties, FIRST\_OBJECT, and END\_OF\_GROUP status objects between moq-transport peers for subgroup 0. Datagrams retain their IDs, properties, priority, and status. Plain Rust readers and moq-lite peers see consecutive frames and normal datagram payloads; JavaScript supports only normal datagrams at Object ID 0. A malformed datagram closes the session; a relayed one the downstream draft cannot carry is dropped. A relayed Timestamp leaves in the units the downstream response declares. A group cannot resume across route changes by frame ordinal once Object IDs skip ahead of the ordinals on either route; recovery refuses it instead of splicing different objects. - **Priority.** Higher is served first. The IETF default of 128 is this stack's 127, and a track that never sets one is 127. - **Size.** An object extension block larger than 64 KiB ends that subgroup stream. The session stays up. This cap is ours, not the draft's. diff --git a/rs/moq-net/src/ietf/publisher.rs b/rs/moq-net/src/ietf/publisher.rs index 41884a86be0..69aa6a0569d 100644 --- a/rs/moq-net/src/ietf/publisher.rs +++ b/rs/moq-net/src/ietf/publisher.rs @@ -142,8 +142,11 @@ async fn read_fetch( if wire.as_ref().is_some_and(|wire| !wire.fetchable) { return Err(Error::Unsupported); } + // Relayed Object IDs need not match frame ordinals, so a relayed group is read from + // its first frame and filtered by ID. Its marked gaps cover the IDs between where it + // starts and its first object; a start below that misses to the upstream. let frame_start = match (&wire, &cached) { - (Some(wire), Some(group)) if skip >= wire.first_id => group.index(), + (Some(_), Some(group)) => skip.min(group.first_frame()), _ => skip, }; let mut group = track diff --git a/rs/moq-net/src/ietf/subscriber.rs b/rs/moq-net/src/ietf/subscriber.rs index 978bb24b4c2..fc542a5af31 100644 --- a/rs/moq-net/src/ietf/subscriber.rs +++ b/rs/moq-net/src/ietf/subscriber.rs @@ -8727,7 +8727,7 @@ mod stitch_tests { for marked in [false, true] { let properties = if marked { vec![0x3e, 6] } else { vec![] }; let second = if marked { vec![0x3e, 1] } else { vec![] }; - for start in [0, 7] { + for start in [0, 2, 7] { let script = object_stream(version, true, &[(6, &properties, None), (8, &second, None)]); let (bytes, log) = relay_passthrough(script, version, version, Some((start, 9))).await; let mut decoder = Decoder::new(&bytes, version.into()); diff --git a/rs/moq-net/src/model/frame.rs b/rs/moq-net/src/model/frame.rs index 061052adb77..c6717803e69 100644 --- a/rs/moq-net/src/model/frame.rs +++ b/rs/moq-net/src/model/frame.rs @@ -807,6 +807,11 @@ impl Consumer { }; } }; + // This copy was dense through its frame, so the replacement's must be the same + // Object ID: a copy that begins past a gap holds different content there. + if frame.fields.as_ref().is_some_and(|fields| fields.id != *index) { + return Poll::Ready(Err(Error::Unsupported)); + } // Same name, same content: a different size is the routes disagreeing. if frame.info.size != self.info.size { return Poll::Ready(Err(Error::ProtocolViolation)); diff --git a/rs/moq-net/src/model/group.rs b/rs/moq-net/src/model/group.rs index b78ff8f023e..84cbe0d4c40 100644 --- a/rs/moq-net/src/model/group.rs +++ b/rs/moq-net/src/model/group.rs @@ -612,7 +612,8 @@ impl Producer { if wire.last.is_some() && gap.is_some_and(|gap| gap > missing) { return Err(Error::MalformedTrack); } - wire.fetchable &= missing == 0 || gap == Some(missing); + // A first object's gap may also cover IDs below where this copy starts. + wire.fetchable &= missing == 0 || gap.is_some_and(|gap| gap >= missing); } Ok(()) } @@ -1232,6 +1233,7 @@ impl Producer { expiry: None, expired: false, ended: false, + adopted: false, stale_counted: Arc::default(), recover: None, } @@ -1495,6 +1497,9 @@ pub struct Consumer { // clean end into `Error::Old` on the next poll, and a caller is allowed to probe // again after the end. ended: bool, + // A replacement copy was adopted and has not yet returned a frame. Its ordinals + // match this group's only while its Object IDs do. + adopted: bool, // Cloned cursors are parallel views of one handed-out delivery. Whichever // observes expiry first records its unread tail; the others must not repeat it. stale_counted: Arc, @@ -1566,6 +1571,7 @@ impl Clone for Consumer { expiry: self.expiry.clone(), expired: self.expired, ended: self.ended, + adopted: self.adopted, stale_counted: self.stale_counted.clone(), recover: self.recover.clone(), } @@ -1595,6 +1601,12 @@ impl Consumer { self.cursor.state.read().wire().cloned() } + /// The index of the first frame the group holds (see [`Producer::start_at`]). + #[cfg(feature = "async")] + pub(crate) fn first_frame(&self) -> u64 { + self.cursor.state.read().offset as u64 + } + #[cfg(feature = "async")] pub(crate) fn end_object(&self) -> Option> { self.cursor.state.read().relay.as_ref()?.end_object.clone() @@ -1663,7 +1675,13 @@ impl Consumer { return res; }; let failed = match &res { - Poll::Ready(Ok(Some(_))) => return res, + Poll::Ready(Ok(Some(_))) => { + // The old copy was dense through this frame, so the replacement must be too. + if std::mem::take(&mut self.adopted) && self.cursor.state.read().sparse() { + return Poll::Ready(Err(Error::Unsupported)); + } + return res; + } // Read to its end: no route is needed for it any more. Poll::Ready(Ok(None)) => { self.recover = None; @@ -1692,6 +1710,7 @@ impl Consumer { self.cursor = replacement.group.cursor; self.cursor.index = index; self.cursor.end = end; + self.adopted = true; } } @@ -2370,6 +2389,28 @@ mod test { assert_eq!(pool.used(), 0); } + #[test] + #[cfg(feature = "async")] + fn a_leading_gap_may_reach_below_where_the_copy_starts() { + for (gap, fetchable) in [(Some(6), true), (Some(4), true), (Some(3), false), (None, false)] { + let mut group = Info { sequence: 0 }.produce(); + group.start_at(2).unwrap(); + group + .set_wire(Wire { + sparse: true, + first_object: false, + first_id: 6, + extensions: true, + has_end: true, + fetchable: true, + last: None, + }) + .unwrap(); + group.mark_gap(6, gap).unwrap(); + assert_eq!(group.wire().unwrap().fetchable, fetchable, "{gap:?}"); + } + } + #[test] #[cfg(feature = "async")] fn demand_watches_readers_without_keeping_the_group_alive() { diff --git a/rs/moq-net/src/model/resume.rs b/rs/moq-net/src/model/resume.rs index 4e0985f884a..4dc480272b0 100644 --- a/rs/moq-net/src/model/resume.rs +++ b/rs/moq-net/src/model/resume.rs @@ -248,7 +248,7 @@ struct Copy { datagrams_done: Option>, /// The newest datagram handed out before this copy was subscribed: a copy that /// buffers datagrams replays them, and those were already handed out. - datagrams_seen: Option, + datagrams_seen: Option<(u64, u64)>, /// Held by every group handed out from this copy, so a replaced copy stays /// subscribed while one is still read. lease: Arc<()>, @@ -443,8 +443,8 @@ struct Reader { copies: Vec, /// The newest group handed out: where a new copy is asked to start. newest: Option, - /// The newest datagram handed out. - datagram: Option, + /// The newest datagram handed out, by Group and Object ID. + datagram: Option<(u64, u64)>, /// The local group filter; see [`track::Subscriber::set_groups`]. groups: (u64, Cap), } @@ -704,10 +704,15 @@ impl Reader { match sub.poll_recv_wire_datagram(waiter) { Poll::Pending => break, Poll::Ready(Ok(Some(datagram))) => { - if copy.datagrams_seen.is_some_and(|seen| datagram.sequence <= seen) { + // A relayed group can carry several datagrams, told apart by Object ID. + let id = ( + datagram.sequence, + datagram.wire.as_ref().and_then(|wire| wire.object_id).unwrap_or(0), + ); + if copy.datagrams_seen.is_some_and(|seen| id <= seen) { continue; } - self.datagram = self.datagram.max(Some(datagram.sequence)); + self.datagram = self.datagram.max(Some(id)); return Some(Poll::Ready(Ok(Some(datagram)))); } Poll::Ready(Ok(None)) => { @@ -1726,6 +1731,45 @@ mod test { assert_eq!(recv(&mut sub), None); } + #[test] + #[cfg(feature = "async")] + fn a_new_copy_hands_out_later_objects_of_a_seen_group() { + let object = |id: u64| super::super::datagram::Stored { + datagram: super::super::datagram::Datagram { + sequence: 1, + timestamp: Some(ts(id)), + payload: bytes::Bytes::from_static(b"d"), + }, + wire: Some(Arc::new(super::super::datagram::Wire { + object_id: Some(id), + priority: None, + end_of_group: false, + status: None, + properties: Default::default(), + })), + }; + let routes = Producer::new(); + let front = logical(&routes); + let mut a = copy(); + routes.serve(a.consume()); + let mut sub = subscribe(&front, Duration::from_secs(10)); + let mut b = copy(); + a.insert_wire_datagram(object(6)).unwrap(); + b.insert_wire_datagram(object(6)).unwrap(); + b.insert_wire_datagram(object(8)).unwrap(); + + let mut recv = || { + sub.poll_recv_wire_datagram(&kio::Waiter::noop()).map(|datagram| { + let datagram = datagram.unwrap().unwrap(); + datagram.wire.as_ref().unwrap().object_id.unwrap() + }) + }; + assert_eq!(recv(), Poll::Ready(6)); + routes.serve(b.consume()); + assert_eq!(recv(), Poll::Ready(8)); + assert!(recv().is_pending()); + } + #[derive(Default)] struct Wake(std::sync::atomic::AtomicBool); @@ -2130,6 +2174,96 @@ mod test { )); } + /// A replacement copy that joined past a gap, holding the group from Object 6. + #[cfg(feature = "async")] + fn joined_past_a_gap(objects: &[(u64, &'static [u8])]) -> track::Producer { + let b = copy(); + let mut group = b.create_group(group::Info { sequence: 0 }).unwrap(); + group + .set_wire(group::Wire { + sparse: false, + first_object: false, + first_id: 6, + extensions: false, + has_end: true, + fetchable: false, + last: None, + }) + .unwrap(); + for &(id, payload) in objects { + let mut frame = group + .create_frame_owned( + crate::frame::Header { + info: crate::frame::Info { + size: payload.len() as u64, + timestamp: Some(ts(id)), + }, + fields: Some(Arc::new(super::super::object::Fields { + id, + properties: Default::default(), + })), + }, + &crate::frame::Budget::default(), + ) + .unwrap(); + frame.write(bytes::Bytes::from_static(payload)).unwrap(); + frame.finish().unwrap(); + } + b + } + + #[test] + #[cfg(feature = "async")] + fn a_replacement_past_a_gap_refuses_ordinal_recovery() { + // A half-read Object 0 must not continue with Object 6's bytes. + let routes = Producer::new(); + let front = logical(&routes); + let a = copy(); + routes.serve(a.consume()); + let mut sub = subscribe(&front, Duration::from_secs(10)); + let mut open = a.create_group(group::Info { sequence: 0 }).unwrap(); + let mut frame = open + .create_frame(crate::frame::Info { + size: 4, + timestamp: Some(ts(0)), + }) + .unwrap(); + frame.write(b"ab".as_ref()).unwrap(); + let mut reading = recv(&mut sub); + let mut chunks = reading.next_frame().now_or_never().unwrap().unwrap().unwrap(); + assert_eq!( + &chunks.read_chunk().now_or_never().unwrap().unwrap().unwrap()[..], + b"ab" + ); + drop(frame); + a.clone().abort(Error::Dropped).unwrap(); + let b = joined_past_a_gap(&[(6, b"WXYZ")]); + routes.serve(b.consume()); + assert!(matches!( + chunks.read_chunk().now_or_never(), + Some(Err(Error::Unsupported)) + )); + + // Ordinal 2 of that copy is Object 8, not Object 2. + let routes = Producer::new(); + let front = logical(&routes); + let a = copy(); + routes.serve(a.consume()); + let mut sub = subscribe(&front, Duration::from_secs(10)); + let mut open = a.create_group(group::Info { sequence: 0 }).unwrap(); + open.write_frame(ts(0), b"a".as_ref()).unwrap(); + open.write_frame(ts(1), b"b".as_ref()).unwrap(); + let mut reading = recv(&mut sub); + assert_eq!(read(&mut reading), Some(b"a".to_vec())); + assert_eq!(read(&mut reading), Some(b"b".to_vec())); + let b = joined_past_a_gap(&[(6, b"x"), (7, b"y"), (8, b"z")]); + routes.serve(b.consume()); + assert!(matches!( + reading.read_frame().now_or_never(), + Some(Err(Error::Unsupported)) + )); + } + #[test] fn a_fetched_group_wakes_when_the_route_changes() { let routes = Producer::new(); From 1255b00e1784869a979e28a47f26390264a62811 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 14 Oct 2026 17:54:36 -0700 Subject: [PATCH 09/12] fix(net): keep refusing an incompatible recovered copy A batch read that already holds frames swallows one error, so a refusal that cleared itself let the next batch continue on a replacement copy whose Object IDs had diverged from its ordinals. The refusal now stands until a frame from that copy passes the check. Co-Authored-By: Claude Opus 5.5 --- rs/moq-net/src/model/group.rs | 12 ++++++++---- rs/moq-net/src/model/resume.rs | 23 +++++++++++++++++++++++ 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/rs/moq-net/src/model/group.rs b/rs/moq-net/src/model/group.rs index 84cbe0d4c40..0339b539756 100644 --- a/rs/moq-net/src/model/group.rs +++ b/rs/moq-net/src/model/group.rs @@ -1497,8 +1497,8 @@ pub struct Consumer { // clean end into `Error::Old` on the next poll, and a caller is allowed to probe // again after the end. ended: bool, - // A replacement copy was adopted and has not yet returned a frame. Its ordinals - // match this group's only while its Object IDs do. + // A replacement copy was adopted and has not yet returned a frame that matches. Its + // ordinals match this group's only while its Object IDs do. adopted: bool, // Cloned cursors are parallel views of one handed-out delivery. Whichever // observes expiry first records its unread tail; the others must not repeat it. @@ -1677,8 +1677,12 @@ impl Consumer { let failed = match &res { Poll::Ready(Ok(Some(_))) => { // The old copy was dense through this frame, so the replacement must be too. - if std::mem::take(&mut self.adopted) && self.cursor.state.read().sparse() { - return Poll::Ready(Err(Error::Unsupported)); + // The refusal stays: a batch that already holds frames swallows one error. + if self.adopted { + if self.cursor.state.read().sparse() { + return Poll::Ready(Err(Error::Unsupported)); + } + self.adopted = false; } return res; } diff --git a/rs/moq-net/src/model/resume.rs b/rs/moq-net/src/model/resume.rs index 4dc480272b0..f9fcf6ec695 100644 --- a/rs/moq-net/src/model/resume.rs +++ b/rs/moq-net/src/model/resume.rs @@ -2262,6 +2262,29 @@ mod test { reading.read_frame().now_or_never(), Some(Err(Error::Unsupported)) )); + + // A batch holding the old copy's frame hides the refusal, but the next one sees it. + let routes = Producer::new(); + let front = logical(&routes); + let a = copy(); + routes.serve(a.consume()); + let mut sub = subscribe(&front, Duration::from_secs(10)); + let mut open = a.create_group(group::Info { sequence: 0 }).unwrap(); + open.write_frame(ts(0), b"a".as_ref()).unwrap(); + let mut reading = recv(&mut sub); + let b = joined_past_a_gap(&[(6, b"x"), (7, b"y"), (8, b"z")]); + routes.serve(b.consume()); + let mut batch = crate::frame::Buffer::<4>::new(); + let waiter = kio::Waiter::noop(); + assert!(matches!( + reading.poll_read_frames(&waiter, &mut batch), + Poll::Ready(Ok(1)) + )); + assert_eq!(&batch.filled_mut()[0].payload[..], b"a"); + assert!(matches!( + reading.poll_read_frames(&waiter, &mut batch), + Poll::Ready(Err(Error::Unsupported)) + )); } #[test] From 23b50d7e702fa664c47342be66b06572bde70cac Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 14 Oct 2026 20:00:00 -0700 Subject: [PATCH 10/12] fix(net): carry END_OF_GROUP through FETCH and drop unitless Timestamps A FETCH discarded a cached group's END_OF_GROUP object, so drafts 14 and 15 lost its ID and properties and the response could end before it. The object now counts toward the fetched group's end on every draft, and drafts 14 and 15 carry it as a status object. A timed ingress relayed to an egress that declares no units forwarded raw Timestamps the receiver cannot interpret. Mutable ones are now dropped, as a plain frame there carries none, and an immutable one is refused. Co-Authored-By: Claude Opus 5.5 --- doc/concept/standard.md | 2 +- rs/moq-net/src/ietf/object.rs | 38 +++++++++++++++++++++--------- rs/moq-net/src/ietf/publisher.rs | 39 +++++++++++++++++++++++++++---- rs/moq-net/src/ietf/subscriber.rs | 39 +++++++++++++++++++++++++++++++ 4 files changed, 101 insertions(+), 17 deletions(-) diff --git a/doc/concept/standard.md b/doc/concept/standard.md index e11967a8f26..c1ee27e86f8 100644 --- a/doc/concept/standard.md +++ b/doc/concept/standard.md @@ -31,7 +31,7 @@ subset drops. What a peer actually observes against this implementation: - **Strict SETUP and parameters.** Repeated unknown `SETUP` options, GREASE included, are accepted if well-formed; a repeated known option is rejected. On draft 17 and later, a `GROUP_ORDER` other than Ascending (1) or Descending (2) closes the session. Message parameters follow the negotiated draft's lists: drafts 14 and 15 ignore an unknown or misplaced parameter, draft 16 ignores one defined only for another message but closes the session on an unknown one, and draft 17 and later close it on both. - **One `SETUP` credential**, forwarded to the [auth server](/bin/relay/auth#the-contract) unverified. A second `SETUP` token closes the session. We advertise no token cache, so an alias reference in `SETUP` closes it with `PROTOCOL_VIOLATION`, one on a request with `UNKNOWN_AUTH_TOKEN_ALIAS`, and a registration on a request with `AUTH_TOKEN_CACHE_OVERFLOW`. A token attached to an individual request is otherwise decoded and grants nothing, and a request repeating one is refused with `MALFORMED_AUTH_TOKEN`. `SUBSCRIBE_TRACKS` is refused unread, so its token is not decoded. On draft 17 and later, the negotiated [MoQ Auth extension](/concept/moq-lite#authorization) accepts additional session tokens in band. - **Refused, not fatal.** A legal request this stack does not serve is rejected on its own and the session stays up: `FORWARD=0`, range filters, `SUBSCRIBE_TRACKS`, and the fetch forms above. On draft 19 and later, and in Rust on drafts 14 through 16, a subscription update may change only priority; any other update ends that subscription. -- **Objects and datagrams.** Rust relays preserve Object IDs, gaps, object properties, FIRST\_OBJECT, and END\_OF\_GROUP status objects between moq-transport peers for subgroup 0. Datagrams retain their IDs, properties, priority, and status. Plain Rust readers and moq-lite peers see consecutive frames and normal datagram payloads; JavaScript supports only normal datagrams at Object ID 0. A malformed datagram closes the session; a relayed one the downstream draft cannot carry is dropped. A relayed Timestamp leaves in the units the downstream response declares. A group cannot resume across route changes by frame ordinal once Object IDs skip ahead of the ordinals on either route; recovery refuses it instead of splicing different objects. +- **Objects and datagrams.** Rust relays preserve Object IDs, gaps, object properties, FIRST\_OBJECT, and END\_OF\_GROUP status objects between moq-transport peers for subgroup 0. Datagrams retain their IDs, properties, priority, and status. Plain Rust readers and moq-lite peers see consecutive frames and normal datagram payloads; JavaScript supports only normal datagrams at Object ID 0. A malformed datagram closes the session; a relayed one the downstream draft cannot carry is dropped. A relayed Timestamp leaves in the units the downstream response declares, and is dropped where it declares none (drafts 14 through 16), as for a plain frame; an immutable one that would change or vanish is refused. A `FETCH` carries the END\_OF\_GROUP object on drafts 14 and 15; later drafts only count it in the response's end. A group cannot resume across route changes by frame ordinal once Object IDs skip ahead of the ordinals on either route; recovery refuses it instead of splicing different objects. - **Priority.** Higher is served first. The IETF default of 128 is this stack's 127, and a track that never sets one is 127. - **Size.** An object extension block larger than 64 KiB ends that subgroup stream. The session stays up. This cap is ours, not the draft's. diff --git a/rs/moq-net/src/ietf/object.rs b/rs/moq-net/src/ietf/object.rs index 28eba0d738e..aee2f51523b 100644 --- a/rs/moq-net/src/ietf/object.rs +++ b/rs/moq-net/src/ietf/object.rs @@ -112,7 +112,9 @@ pub(super) fn write(properties: &Properties, version: Version, timescale: Option { return Err(Error::Unsupported); } - // Without both units there is nothing to convert: an untimed track forwards them as is. + // An untimed ingress forwards its Timestamps as is. A timed one to an egress that + // declares no units drops them, as a plain frame there carries none. + let unstamped = properties.timescale.is_some() && timescale.is_none(); let convert = match (properties.timescale, timescale) { (Some(from), Some(to)) if from != to => Some((from, to)), _ => None, @@ -126,12 +128,14 @@ pub(super) fn write(properties: &Properties, version: Version, timescale: Option None => value, })) }; - // Rewriting an immutable Timestamp would break it; forwarding it would change its meaning. - if properties.sealed_time && convert.is_some() { + // Rewriting or dropping an immutable Timestamp would break it; forwarding it would + // change its meaning. + if properties.sealed_time && (convert.is_some() || unstamped) { return Err(Error::Unsupported); } let timestamp = properties .timestamp + .filter(|_| !unstamped) .map(|value| { Ok::<_, Error>(Property { id: 0x10, @@ -143,7 +147,7 @@ pub(super) fn write(properties: &Properties, version: Version, timescale: Option let legacy = properties .values .iter() - .filter(|property| property.id == 0x06) + .filter(|property| property.id == 0x06 && !unstamped) .map(|property| match property.value { Value::Int(value) => Ok(Property { id: 0x06, @@ -231,11 +235,16 @@ mod tests { write(&decoded.properties, Version::Draft17, milli), Err(Error::Unsupported) )); - // Its units cannot follow the egress's without rewriting the block. - assert!(matches!( - write(&decoded.properties, Version::Draft18, Some(Timescale::MICRO)), - Err(Error::Unsupported) - )); + // Its units cannot follow the egress's, or vanish with them, without rewriting the block. + for timescale in [Some(Timescale::MICRO), None] { + assert!(matches!( + write(&decoded.properties, Version::Draft18, timescale), + Err(Error::Unsupported) + )); + } + // An untimed ingress has no units to lose. + let untimed = read(&bytes, Version::Draft22, None).unwrap(); + assert_eq!(write(&untimed.properties, Version::Draft18, None).unwrap(), bytes); } /// The cache keeps a timed track in its own units, so a relayed Timestamp must leave @@ -251,8 +260,15 @@ mod tests { assert_eq!(again.timestamp, Some(micros), "key {key:#x}"); assert_eq!(again.properties.values.last().unwrap().value, Value::Int(7)); - // An egress that declares no units forwards the value untouched. - assert_eq!(write(&decoded.properties, Version::Draft22, None).unwrap(), bytes); + // An egress that declares no units drops it, as a plain frame there has none. + let written = write(&decoded.properties, Version::Draft22, None).unwrap(); + let again = read(&Bytes::from(written), Version::Draft22, None).unwrap(); + assert_eq!(again.properties.timestamp, None, "key {key:#x}"); + assert_eq!(again.properties.values.len(), 1, "key {key:#x}"); + assert_eq!(again.properties.values[0].value, Value::Int(7)); + // An untimed ingress forwards it untouched. + let untimed = read(&bytes, Version::Draft22, None).unwrap(); + assert_eq!(write(&untimed.properties, Version::Draft22, None).unwrap(), bytes); } } diff --git a/rs/moq-net/src/ietf/publisher.rs b/rs/moq-net/src/ietf/publisher.rs index 69aa6a0569d..7610f797780 100644 --- a/rs/moq-net/src/ietf/publisher.rs +++ b/rs/moq-net/src/ietf/publisher.rs @@ -80,6 +80,8 @@ struct FetchedGroup { /// The Object ID of the first frame. first: u64, frames: Vec, + /// The group's END_OF_GROUP status object, when it falls in the range. + end_object: Option>, /// The fetched range reached the finished group's final object. complete: bool, /// The track's info, which FETCH_OK describes and whose units stamp the objects. @@ -111,8 +113,11 @@ fn fetch_prior(version: Version, prior: Option, object: u64) -> Result u64 { + if let Some(end) = &self.end_object { + return end.id.saturating_add(1); + } self.frames.last().map_or(self.first, |frame| { frame .fields @@ -188,6 +193,9 @@ async fn read_fetch( let complete = !overshot && matches!(group.finished().now_or_never(), Some(Ok(_))) && group.index() >= group.frame_count() as u64; + let end_object = group + .end_object() + .filter(|end| complete && end.id >= skip && until.is_none_or(|until| end.id < until)); if group.wire().is_some_and(|wire| !wire.fetchable) { return Err(Error::Unsupported); @@ -196,6 +204,7 @@ async fn read_fetch( sequence, first, frames, + end_object, complete, info, }) @@ -1779,10 +1788,9 @@ where .info .timescale .filter(|_| ietf::Properties::sends_timescale(self.version)); - for frame in &group.frames { - if let Some(fields) = &frame.fields - && let Err(err) = super::object::write(&fields.properties, self.version, timescale) - { + let fields = group.frames.iter().filter_map(|frame| frame.fields.as_deref()); + for fields in fields.chain(group.end_object.as_deref()) { + if let Err(err) = super::object::write(&fields.properties, self.version, timescale) { return self.reject_fetch(stream, msg.request_id, &err, &err.to_string()).await; } } @@ -1916,6 +1924,26 @@ where .await?; Self::write_fetch_payload(&mut writer, frame.frame.payload, self.version).await?; } + // Only draft-14 and 15 FETCH objects carry a status; later drafts imply the end. + if let Some(end) = group.end_object.as_deref() + && matches!(self.version, Version::Draft14 | Version::Draft15) + { + Self::write_fetch_object( + &mut writer, + FetchObjectInfo { + sequence: group.sequence, + object: end.id, + prior: fetch_prior(self.version, prior, end.id)?, + timestamp: None, + timescale, + version: self.version, + fields: Some(end), + }, + ) + .await?; + writer.varint(0).await?; + writer.varint(END_OF_GROUP).await?; + } settle(&mut writer).await?; Ok::<(), Error>(()) }; @@ -3916,6 +3944,7 @@ impl GroupServe { } /// Object status: no object at or past this location exists. +const END_OF_GROUP: u64 = 0x3; const END_OF_TRACK: u64 = 0x4; /// An object's header, with private relay fields when it came from an IETF peer. diff --git a/rs/moq-net/src/ietf/subscriber.rs b/rs/moq-net/src/ietf/subscriber.rs index fc542a5af31..0eff49e74b6 100644 --- a/rs/moq-net/src/ietf/subscriber.rs +++ b/rs/moq-net/src/ietf/subscriber.rs @@ -8777,6 +8777,45 @@ mod stitch_tests { } } + /// A relayed END_OF_GROUP object ends the fetched group, and drafts 14 and 15 carry it. + #[moq_net_sim::test] + async fn passthrough_fetch_keeps_the_end_of_group_object() { + for version in [Version::Draft15, Version::Draft22] { + let objects = [ + (6, &[0x3e, 6][..], None), + (8, &[0x3e, 1][..], None), + (11, &[0x3e, 2][..], Some(END_OF_GROUP)), + ]; + let script = object_stream(version, true, &objects); + let (bytes, log) = relay_passthrough(script, version, version, Some((0, 12))).await; + let mut decoder = Decoder::new(&bytes, version.into()); + assert_eq!(decoder.varint().unwrap(), ietf::FetchOk::ID, "{version}"); + let ok = ietf::FetchOk::decode(&mut decoder, version).unwrap(); + let last = if Filter::is_draft20(version) { 11 } else { 12 }; + assert_eq!(ok.end_location.object, last, "{version}"); + assert_eq!(decoder.varint().unwrap(), ietf::FetchHeader::TYPE); + ietf::FetchHeader::decode(&mut decoder, version).unwrap(); + for _ in [6, 8] { + ietf::FetchObject::decode(&mut decoder, version).unwrap(); + assert_eq!(decoder.varint().unwrap(), 1); + assert_eq!(decoder.slice(1).unwrap(), &[99]); + } + if version == Version::Draft15 { + let ietf::FetchObject::Object { object, properties, .. } = + ietf::FetchObject::decode(&mut decoder, version).unwrap() + else { + panic!("status object"); + }; + assert_eq!(object, Some(11)); + assert_eq!(properties.as_deref(), Some(&[0x3e, 2][..])); + assert_eq!(decoder.varint().unwrap(), 0); + assert_eq!(decoder.varint().unwrap(), END_OF_GROUP); + } + assert!(decoder.is_empty(), "{version}"); + assert!(log.resets().is_empty()); + } + } + #[moq_net_sim::test] async fn passthrough_refuses_contradictory_gap_properties() { for properties in [vec![0x3e, 2], vec![0x3e, 3], vec![0x3e, 1, 0, 1]] { From 3908bde37de3a814403b8a57b058e96b142bb991 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 14 Oct 2026 20:46:34 -0700 Subject: [PATCH 11/12] fix(net): agree on a fetched group's end and drop refused prefetch A FETCH counted the END_OF_GROUP object in its end on drafts that cannot carry it, and a receiving relay never counted one it did receive, so the next relay rejected the clean FIN as short of the End Location. Only drafts 14 and 15 keep the object, and the receiver counts its Object ID. A refused recovered copy left its prefetched frames for read_frame's fast path, which hands them out without the check. The refusal now drops them. Co-Authored-By: Claude Opus 5.5 --- doc/concept/standard.md | 2 +- rs/moq-net/src/ietf/publisher.rs | 12 ++-- rs/moq-net/src/ietf/subscriber.rs | 93 ++++++++++++++++++++----------- rs/moq-net/src/model/group.rs | 2 + rs/moq-net/src/model/resume.rs | 51 +++++++++-------- 5 files changed, 100 insertions(+), 60 deletions(-) diff --git a/doc/concept/standard.md b/doc/concept/standard.md index c1ee27e86f8..76474f67c2a 100644 --- a/doc/concept/standard.md +++ b/doc/concept/standard.md @@ -31,7 +31,7 @@ subset drops. What a peer actually observes against this implementation: - **Strict SETUP and parameters.** Repeated unknown `SETUP` options, GREASE included, are accepted if well-formed; a repeated known option is rejected. On draft 17 and later, a `GROUP_ORDER` other than Ascending (1) or Descending (2) closes the session. Message parameters follow the negotiated draft's lists: drafts 14 and 15 ignore an unknown or misplaced parameter, draft 16 ignores one defined only for another message but closes the session on an unknown one, and draft 17 and later close it on both. - **One `SETUP` credential**, forwarded to the [auth server](/bin/relay/auth#the-contract) unverified. A second `SETUP` token closes the session. We advertise no token cache, so an alias reference in `SETUP` closes it with `PROTOCOL_VIOLATION`, one on a request with `UNKNOWN_AUTH_TOKEN_ALIAS`, and a registration on a request with `AUTH_TOKEN_CACHE_OVERFLOW`. A token attached to an individual request is otherwise decoded and grants nothing, and a request repeating one is refused with `MALFORMED_AUTH_TOKEN`. `SUBSCRIBE_TRACKS` is refused unread, so its token is not decoded. On draft 17 and later, the negotiated [MoQ Auth extension](/concept/moq-lite#authorization) accepts additional session tokens in band. - **Refused, not fatal.** A legal request this stack does not serve is rejected on its own and the session stays up: `FORWARD=0`, range filters, `SUBSCRIBE_TRACKS`, and the fetch forms above. On draft 19 and later, and in Rust on drafts 14 through 16, a subscription update may change only priority; any other update ends that subscription. -- **Objects and datagrams.** Rust relays preserve Object IDs, gaps, object properties, FIRST\_OBJECT, and END\_OF\_GROUP status objects between moq-transport peers for subgroup 0. Datagrams retain their IDs, properties, priority, and status. Plain Rust readers and moq-lite peers see consecutive frames and normal datagram payloads; JavaScript supports only normal datagrams at Object ID 0. A malformed datagram closes the session; a relayed one the downstream draft cannot carry is dropped. A relayed Timestamp leaves in the units the downstream response declares, and is dropped where it declares none (drafts 14 through 16), as for a plain frame; an immutable one that would change or vanish is refused. A `FETCH` carries the END\_OF\_GROUP object on drafts 14 and 15; later drafts only count it in the response's end. A group cannot resume across route changes by frame ordinal once Object IDs skip ahead of the ordinals on either route; recovery refuses it instead of splicing different objects. +- **Objects and datagrams.** Rust relays preserve Object IDs, gaps, object properties, FIRST\_OBJECT, and END\_OF\_GROUP status objects between moq-transport peers for subgroup 0. Datagrams retain their IDs, properties, priority, and status. Plain Rust readers and moq-lite peers see consecutive frames and normal datagram payloads; JavaScript supports only normal datagrams at Object ID 0. A malformed datagram closes the session; a relayed one the downstream draft cannot carry is dropped. A relayed Timestamp leaves in the units the downstream response declares, and is dropped where it declares none (drafts 14 through 16), as for a plain frame; an immutable one that would change or vanish is refused. A `FETCH` carries the END\_OF\_GROUP object on drafts 14 and 15; later drafts have no FETCH status, so their response ends at the last frame. A group cannot resume across route changes by frame ordinal once Object IDs skip ahead of the ordinals on either route; recovery refuses it instead of splicing different objects. - **Priority.** Higher is served first. The IETF default of 128 is this stack's 127, and a track that never sets one is 127. - **Size.** An object extension block larger than 64 KiB ends that subgroup stream. The session stays up. This cap is ours, not the draft's. diff --git a/rs/moq-net/src/ietf/publisher.rs b/rs/moq-net/src/ietf/publisher.rs index 7610f797780..f6b862e9faa 100644 --- a/rs/moq-net/src/ietf/publisher.rs +++ b/rs/moq-net/src/ietf/publisher.rs @@ -1777,11 +1777,16 @@ where }) .await }; - let group = match group { + let mut group = match group { Some(Ok(group)) => group, Some(Err(err)) => return self.reject_fetch(stream, msg.request_id, &err, &err.to_string()).await, None => return Ok(()), }; + // Only draft-14 and 15 FETCH objects carry a status. Later drafts end the response at + // the last frame, which is all a receiver can account for. + if !matches!(self.version, Version::Draft14 | Version::Draft15) { + group.end_object = None; + } // FETCH keeps each object's Timestamp, in the units FETCH_OK declares wherever the // draft can declare them. Opting out of the properties strips nothing from the objects. let timescale = group @@ -1924,10 +1929,7 @@ where .await?; Self::write_fetch_payload(&mut writer, frame.frame.payload, self.version).await?; } - // Only draft-14 and 15 FETCH objects carry a status; later drafts imply the end. - if let Some(end) = group.end_object.as_deref() - && matches!(self.version, Version::Draft14 | Version::Draft15) - { + if let Some(end) = group.end_object.as_deref() { Self::write_fetch_object( &mut writer, FetchObjectInfo { diff --git a/rs/moq-net/src/ietf/subscriber.rs b/rs/moq-net/src/ietf/subscriber.rs index 0eff49e74b6..ac3c0546678 100644 --- a/rs/moq-net/src/ietf/subscriber.rs +++ b/rs/moq-net/src/ietf/subscriber.rs @@ -3787,7 +3787,7 @@ where producer.start_at(resume.frame)?; } let keep = *next >= producer.frame_count() as u64; - if !self + if self .recv_fetch_payload( stream, producer, @@ -3798,7 +3798,7 @@ where id, }, ) - .await? + .await? != Fetched::Frame { return Err(Error::Unsupported); } @@ -3810,15 +3810,12 @@ where /// Read one fetch object's length and payload into `producer`, after its header, or /// past it unless `keep`. - /// - /// Returns `false` for a draft-14 or 15 end-of-group or end-of-track marker, which - /// is a status rather than a frame. async fn recv_fetch_payload( &self, stream: &mut Reader, producer: &mut group::Producer, payload: FetchPayload, - ) -> Result { + ) -> Result { let FetchPayload { decoded, timescale, @@ -3839,16 +3836,16 @@ where properties: decoded.properties, })?; } - return Ok(false); + return Ok(Fetched::EndOfGroup); } - END_OF_TRACK => return Ok(false), + END_OF_TRACK => return Ok(Fetched::EndOfTrack), _ => return Err(Error::Unsupported), } } if !keep { let mut remaining = usize::try_from(size).map_err(|_| Error::FrameTooLarge)?; std::future::poll_fn(|cx| stream.poll_skip(cx, &mut remaining)).await?; - return Ok(true); + return Ok(Fetched::Frame); } let timestamp = object_time(timescale, decoded.timestamp)?; let plain = id == producer.frame_count() as u64 @@ -3870,7 +3867,7 @@ where return Err(err); } frame.finish()?; - Ok(true) + Ok(Fetched::Frame) } /// Fetch a group from the publisher to fill a cache miss, with a standalone (or @@ -4316,8 +4313,13 @@ where ) .await? { - true => next = id.checked_add(1).ok_or(Error::ProtocolViolation)?, - false => ended = true, + Fetched::Frame => next = id.checked_add(1).ok_or(Error::ProtocolViolation)?, + // The status is an object of the group, so the End Location counts it. + Fetched::EndOfGroup => { + next = id.checked_add(1).ok_or(Error::ProtocolViolation)?; + ended = true; + } + Fetched::EndOfTrack => ended = true, } if end.is_some_and(|end| next > end) { tracing::warn!(sequence, ?end, "a group fetch continued past FETCH_OK's End Location"); @@ -4342,6 +4344,16 @@ where } } +/// What a fetch object carried. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum Fetched { + Frame, + /// A draft-14 or 15 END_OF_GROUP status object. + EndOfGroup, + /// A draft-14 or 15 END_OF_TRACK marker. + EndOfTrack, +} + struct FetchPayload { decoded: super::object::Decoded, timescale: Option, @@ -8777,10 +8789,11 @@ mod stitch_tests { } } - /// A relayed END_OF_GROUP object ends the fetched group, and drafts 14 and 15 carry it. + /// Drafts 14 and 15 carry a relayed END_OF_GROUP object through FETCH, and the next + /// relay fills its cache from that answer. Later drafts end the answer at the last frame. #[moq_net_sim::test] async fn passthrough_fetch_keeps_the_end_of_group_object() { - for version in [Version::Draft15, Version::Draft22] { + for version in [Version::Draft15, Version::Draft16, Version::Draft17, Version::Draft22] { let objects = [ (6, &[0x3e, 6][..], None), (8, &[0x3e, 1][..], None), @@ -8788,31 +8801,47 @@ mod stitch_tests { ]; let script = object_stream(version, true, &objects); let (bytes, log) = relay_passthrough(script, version, version, Some((0, 12))).await; + assert!(log.resets().is_empty()); let mut decoder = Decoder::new(&bytes, version.into()); assert_eq!(decoder.varint().unwrap(), ietf::FetchOk::ID, "{version}"); let ok = ietf::FetchOk::decode(&mut decoder, version).unwrap(); - let last = if Filter::is_draft20(version) { 11 } else { 12 }; + let carried = version == Version::Draft15; + let last = match () { + _ if carried => 12, + _ if Filter::is_draft20(version) => 8, + _ => 9, + }; assert_eq!(ok.end_location.object, last, "{version}"); assert_eq!(decoder.varint().unwrap(), ietf::FetchHeader::TYPE); ietf::FetchHeader::decode(&mut decoder, version).unwrap(); - for _ in [6, 8] { - ietf::FetchObject::decode(&mut decoder, version).unwrap(); - assert_eq!(decoder.varint().unwrap(), 1); - assert_eq!(decoder.slice(1).unwrap(), &[99]); - } - if version == Version::Draft15 { - let ietf::FetchObject::Object { object, properties, .. } = - ietf::FetchObject::decode(&mut decoder, version).unwrap() - else { - panic!("status object"); - }; - assert_eq!(object, Some(11)); - assert_eq!(properties.as_deref(), Some(&[0x3e, 2][..])); - assert_eq!(decoder.varint().unwrap(), 0); - assert_eq!(decoder.varint().unwrap(), END_OF_GROUP); + + // The next relay reads the same objects into its cache, up to the same end. + let end = last + u64::from(Filter::is_draft20(version)); + let mut run = GroupFetchRun::new(version, decoder.rest().to_vec()).await; + let mut producer = run.track.create_group(group::Info { sequence: SEQUENCE }).unwrap(); + run.subscriber + .recv_group_fetch_objects(&mut run.stream, &mut producer, 0, Some(end), None) + .await + .unwrap_or_else(|err| panic!("{version}: {err}")); + producer.finish().unwrap(); + let mut reader = producer.consume(); + for id in [6, 8] { + let frame = kio::wait(|waiter| reader.poll_read_object(waiter)) + .await + .unwrap() + .unwrap(); + assert_eq!(frame.fields.as_ref().unwrap().id, id, "{version}"); + } + assert!(reader.read_frame().await.unwrap().is_none()); + let end_object = reader.end_object(); + assert_eq!( + end_object.as_ref().map(|end| end.id), + carried.then_some(11), + "{version}" + ); + if let Some(end) = end_object { + assert_eq!(end.properties.values[0].id, 0x3e); } - assert!(decoder.is_empty(), "{version}"); - assert!(log.resets().is_empty()); } } diff --git a/rs/moq-net/src/model/group.rs b/rs/moq-net/src/model/group.rs index 0339b539756..d86b87ee0ef 100644 --- a/rs/moq-net/src/model/group.rs +++ b/rs/moq-net/src/model/group.rs @@ -1680,6 +1680,8 @@ impl Consumer { // The refusal stays: a batch that already holds frames swallows one error. if self.adopted { if self.cursor.state.read().sparse() { + // `read_frame` pops prefetched frames without coming back here. + self.cursor.prefetch = Prefetch::default(); return Poll::Ready(Err(Error::Unsupported)); } self.adopted = false; diff --git a/rs/moq-net/src/model/resume.rs b/rs/moq-net/src/model/resume.rs index f9fcf6ec695..51e5f021e36 100644 --- a/rs/moq-net/src/model/resume.rs +++ b/rs/moq-net/src/model/resume.rs @@ -2263,28 +2263,35 @@ mod test { Some(Err(Error::Unsupported)) )); - // A batch holding the old copy's frame hides the refusal, but the next one sees it. - let routes = Producer::new(); - let front = logical(&routes); - let a = copy(); - routes.serve(a.consume()); - let mut sub = subscribe(&front, Duration::from_secs(10)); - let mut open = a.create_group(group::Info { sequence: 0 }).unwrap(); - open.write_frame(ts(0), b"a".as_ref()).unwrap(); - let mut reading = recv(&mut sub); - let b = joined_past_a_gap(&[(6, b"x"), (7, b"y"), (8, b"z")]); - routes.serve(b.consume()); - let mut batch = crate::frame::Buffer::<4>::new(); - let waiter = kio::Waiter::noop(); - assert!(matches!( - reading.poll_read_frames(&waiter, &mut batch), - Poll::Ready(Ok(1)) - )); - assert_eq!(&batch.filled_mut()[0].payload[..], b"a"); - assert!(matches!( - reading.poll_read_frames(&waiter, &mut batch), - Poll::Ready(Err(Error::Unsupported)) - )); + // A batch holding the old copy's frame hides the refusal, but the next read sees it, + // whether a batch or a single frame served from what that batch prefetched. + for single in [false, true] { + let routes = Producer::new(); + let front = logical(&routes); + let a = copy(); + routes.serve(a.consume()); + let mut sub = subscribe(&front, Duration::from_secs(10)); + let mut open = a.create_group(group::Info { sequence: 0 }).unwrap(); + open.write_frame(ts(0), b"a".as_ref()).unwrap(); + let mut reading = recv(&mut sub); + let b = joined_past_a_gap(&[(6, b"x"), (7, b"y"), (8, b"z")]); + routes.serve(b.consume()); + let mut batch = crate::frame::Buffer::<4>::new(); + let waiter = kio::Waiter::noop(); + assert!(matches!( + reading.poll_read_frames(&waiter, &mut batch), + Poll::Ready(Ok(1)) + )); + assert_eq!(&batch.filled_mut()[0].payload[..], b"a"); + let next = match single { + true => reading.read_frame().now_or_never().unwrap().map(|_| ()), + false => match reading.poll_read_frames(&waiter, &mut batch) { + Poll::Ready(res) => res.map(|_| ()), + Poll::Pending => panic!("the batch parked"), + }, + }; + assert!(matches!(next, Err(Error::Unsupported)), "single={single}"); + } } #[test] From 796a7142afab79a4cd74c872a144c1108eb19763 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 15 Oct 2026 09:07:28 -0700 Subject: [PATCH 12/12] fix(net): read 0x06 by draft and keep properties off status objects Draft-19 reassigns Object Property 0x06 from the draft-ietf-moq-loc-03 Timestamp to SUBGROUP_DELIVERY_TIMEOUT, so only earlier drafts read it as a time. Across that boundary a mutable 0x06 is dropped and a sealed one refused. JS decodes the same way. From draft 15, only a Normal object may carry properties: a status object with them is refused on ingest, and one relayed from draft 14 leaves without them. Co-Authored-By: Claude Opus 5.5 --- doc/concept/standard.md | 2 +- js/net/src/ietf/ietf.test.ts | 21 ++++++ js/net/src/ietf/object.ts | 17 ++++- rs/moq-net/src/ietf/object.rs | 108 +++++++++++++++++++++++++----- rs/moq-net/src/ietf/publisher.rs | 18 ++++- rs/moq-net/src/ietf/subscriber.rs | 77 ++++++++++++++++++--- rs/moq-net/src/model/object.rs | 8 +++ 7 files changed, 225 insertions(+), 26 deletions(-) diff --git a/doc/concept/standard.md b/doc/concept/standard.md index 76474f67c2a..6cf9fa1467a 100644 --- a/doc/concept/standard.md +++ b/doc/concept/standard.md @@ -31,7 +31,7 @@ subset drops. What a peer actually observes against this implementation: - **Strict SETUP and parameters.** Repeated unknown `SETUP` options, GREASE included, are accepted if well-formed; a repeated known option is rejected. On draft 17 and later, a `GROUP_ORDER` other than Ascending (1) or Descending (2) closes the session. Message parameters follow the negotiated draft's lists: drafts 14 and 15 ignore an unknown or misplaced parameter, draft 16 ignores one defined only for another message but closes the session on an unknown one, and draft 17 and later close it on both. - **One `SETUP` credential**, forwarded to the [auth server](/bin/relay/auth#the-contract) unverified. A second `SETUP` token closes the session. We advertise no token cache, so an alias reference in `SETUP` closes it with `PROTOCOL_VIOLATION`, one on a request with `UNKNOWN_AUTH_TOKEN_ALIAS`, and a registration on a request with `AUTH_TOKEN_CACHE_OVERFLOW`. A token attached to an individual request is otherwise decoded and grants nothing, and a request repeating one is refused with `MALFORMED_AUTH_TOKEN`. `SUBSCRIBE_TRACKS` is refused unread, so its token is not decoded. On draft 17 and later, the negotiated [MoQ Auth extension](/concept/moq-lite#authorization) accepts additional session tokens in band. - **Refused, not fatal.** A legal request this stack does not serve is rejected on its own and the session stays up: `FORWARD=0`, range filters, `SUBSCRIBE_TRACKS`, and the fetch forms above. On draft 19 and later, and in Rust on drafts 14 through 16, a subscription update may change only priority; any other update ends that subscription. -- **Objects and datagrams.** Rust relays preserve Object IDs, gaps, object properties, FIRST\_OBJECT, and END\_OF\_GROUP status objects between moq-transport peers for subgroup 0. Datagrams retain their IDs, properties, priority, and status. Plain Rust readers and moq-lite peers see consecutive frames and normal datagram payloads; JavaScript supports only normal datagrams at Object ID 0. A malformed datagram closes the session; a relayed one the downstream draft cannot carry is dropped. A relayed Timestamp leaves in the units the downstream response declares, and is dropped where it declares none (drafts 14 through 16), as for a plain frame; an immutable one that would change or vanish is refused. A `FETCH` carries the END\_OF\_GROUP object on drafts 14 and 15; later drafts have no FETCH status, so their response ends at the last frame. A group cannot resume across route changes by frame ordinal once Object IDs skip ahead of the ordinals on either route; recovery refuses it instead of splicing different objects. +- **Objects and datagrams.** Rust relays preserve Object IDs, gaps, object properties, FIRST\_OBJECT, and END\_OF\_GROUP status objects between moq-transport peers for subgroup 0. Datagrams retain their IDs, properties, priority, and status. Plain Rust readers and moq-lite peers see consecutive frames and normal datagram payloads; JavaScript supports only normal datagrams at Object ID 0. A malformed datagram closes the session; a relayed one the downstream draft cannot carry is dropped. A relayed Timestamp leaves in the units the downstream response declares, and is dropped where it declares none (drafts 14 through 16), as for a plain frame; an immutable one that would change or vanish is refused. Property 0x06 is a draft-ietf-moq-loc-03 Timestamp only before draft 19, which reassigns it to SUBGROUP\_DELIVERY\_TIMEOUT, so it is dropped across that boundary, or refused when immutable. From draft 15, a status object carrying properties is refused, and one relayed from draft 14 leaves without them. A `FETCH` carries the END\_OF\_GROUP object on drafts 14 and 15; later drafts have no FETCH status, so their response ends at the last frame. A group cannot resume across route changes by frame ordinal once Object IDs skip ahead of the ordinals on either route; recovery refuses it instead of splicing different objects. - **Priority.** Higher is served first. The IETF default of 128 is this stack's 127, and a track that never sets one is 127. - **Size.** An object extension block larger than 64 KiB ends that subgroup stream. The session stays up. This cap is ours, not the draft's. diff --git a/js/net/src/ietf/ietf.test.ts b/js/net/src/ietf/ietf.test.ts index 32a625d4511..25c49e56240 100644 --- a/js/net/src/ietf/ietf.test.ts +++ b/js/net/src/ietf/ietf.test.ts @@ -1857,6 +1857,27 @@ test("Frame rejects a property that runs past its block", async () => { ); }); +// Draft-19 reassigns property 0x06 from the draft-ietf-moq-loc-03 Timestamp to +// SUBGROUP_DELIVERY_TIMEOUT, which must not be read as a time. +test("Frame reads property 0x06 as a Timestamp only before draft-19", async () => { + const flags: GroupFlags = { + hasExtensions: true, + hasSubgroup: false, + hasSubgroupObject: false, + hasEnd: true, + hasPriority: true, + firstObject: true, + }; + for (const [version, expected] of [ + [Version.DRAFT_18, 42], + [Version.DRAFT_19, undefined], + ] as const) { + const reader = new Reader(undefined, new Uint8Array([0, 2, 0x06, 42, 1, 0xaa]), version); + const frame = await reader.decode((c) => Frame.decode(c, flags, Timescale.MILLI)); + expect(frame.timestamp?.value).toBe(expected); + } +}); + // A fetch stream's first object is the only one carrying absolute ids, so a wrong flag byte // there silently renumbers every object after it. This codec is the sole serialization path // for a served fill. diff --git a/js/net/src/ietf/object.ts b/js/net/src/ietf/object.ts index 2b8ecff9750..43ccefae4f3 100644 --- a/js/net/src/ietf/object.ts +++ b/js/net/src/ietf/object.ts @@ -19,6 +19,20 @@ const PROP_TIMESTAMP = 0x10n; // so it is not accepted here. const PROP_TIMESTAMP_DRAFT03 = 0x06n; +/** Whether 0x06 is the draft-03 Timestamp: draft-19 reassigns it to SUBGROUP_DELIVERY_TIMEOUT. */ +function hasDraft03Timestamp(version: IetfVersion | undefined): boolean { + switch (version) { + case Version.DRAFT_14: + case Version.DRAFT_15: + case Version.DRAFT_16: + case Version.DRAFT_17: + case Version.DRAFT_18: + return true; + default: + return false; + } +} + // draft-18 adds bit 0x40 (FIRST_OBJECT) to the subgroup header type per spec // 11.4.2. Set on emit unless a filter trimmed the group's head. On parse a // cleared bit is the peer's claim that the stream starts partway through; @@ -110,6 +124,7 @@ export function decodeObjectTime(c: Cursor, timescale: Timescale): Timestamp | u let timestamp: bigint | undefined; let prevType = 0n; let first = true; + const draft03 = hasDraft03Timestamp(asIetf(c.version)); while (c.remaining > 0) { const step = c.u62(); @@ -119,7 +134,7 @@ export function decodeObjectTime(c: Cursor, timescale: Timescale): Timestamp | u if (id % 2n === 0n) { const value = c.u62(); - if (id === PROP_TIMESTAMP || id === PROP_TIMESTAMP_DRAFT03) { + if (id === PROP_TIMESTAMP || (draft03 && id === PROP_TIMESTAMP_DRAFT03)) { timestamp = value; } } else { diff --git a/rs/moq-net/src/ietf/object.rs b/rs/moq-net/src/ietf/object.rs index aee2f51523b..15f7e6de29d 100644 --- a/rs/moq-net/src/ietf/object.rs +++ b/rs/moq-net/src/ietf/object.rs @@ -16,6 +16,30 @@ pub(super) fn encoding(version: Version) -> Encoding { } } +/// Before draft-19, 0x06 is no Object Property, so a publisher on draft-ietf-moq-loc-03 +/// used it for the Timestamp. Draft-19 assigns it to SUBGROUP_DELIVERY_TIMEOUT. +fn legacy_time(version: Version) -> bool { + matches!( + version, + Version::Draft14 | Version::Draft15 | Version::Draft16 | Version::Draft17 | Version::Draft18 + ) +} + +/// Only draft-14 lets a status object, such as END_OF_GROUP, carry properties. +#[cfg(feature = "async")] +pub(super) fn status_has_properties(version: Version) -> bool { + version == Version::Draft14 +} + +/// Refuse properties on a status object where its draft forbids them. +#[cfg(feature = "async")] +pub(super) fn check_status(version: Version, properties: &Properties) -> Result<(), Error> { + match status_has_properties(version) || properties.is_empty() { + true => Ok(()), + false => Err(Error::Session(crate::SessionError::ProtocolViolation)), + } +} + pub(super) struct Decoded { pub properties: Properties, pub timestamp: Option, @@ -48,7 +72,7 @@ fn list(data: &Bytes, version: Version, immutable: bool) -> Result previous = id; let value = if id % 2 == 0 { let value = decoder.varint()?; - if matches!(id, 0x10 | 0x06) { + if id == 0x10 || (id == 0x06 && legacy_time(version)) { result.time = Some(value); } if id == 0x3e && result.gap.replace(value).is_some() { @@ -92,6 +116,7 @@ pub(super) fn read(data: &Bytes, version: Version, timescale: Option) let mut parsed = list(data, version, false)?; parsed.properties.timescale = timescale.filter(|_| parsed.time.is_some()); parsed.properties.sealed_time = parsed.sealed_time; + parsed.properties.legacy_time = legacy_time(version); let timestamp = match (parsed.time, timescale) { (Some(value), Some(timescale)) => Some(Timestamp::new(value, timescale).map_err(|_| Error::MalformedTrack)?), _ => None, @@ -133,6 +158,12 @@ pub(super) fn write(properties: &Properties, version: Version, timescale: Option if properties.sealed_time && (convert.is_some() || unstamped) { return Err(Error::Unsupported); } + // Across draft-19, 0x06 turns from a Timestamp into SUBGROUP_DELIVERY_TIMEOUT or back, + // so neither reading survives the hop. A mutable one is dropped; a sealed one refused. + let flipped = properties.legacy_time != legacy_time(version); + if flipped && sealed_key(properties, version, 0x06)? { + return Err(Error::Unsupported); + } let timestamp = properties .timestamp .filter(|_| !unstamped) @@ -143,17 +174,18 @@ pub(super) fn write(properties: &Properties, version: Version, timescale: Option }) }) .transpose()?; - // The draft-03 Timestamp key carries the same units. + // The draft-03 Timestamp key carries the same units. Later, it is opaque. let legacy = properties .values .iter() - .filter(|property| property.id == 0x06 && !unstamped) - .map(|property| match property.value { - Value::Int(value) => Ok(Property { + .filter(|property| property.id == 0x06 && !flipped && !(properties.legacy_time && unstamped)) + .map(|property| match (properties.legacy_time, &property.value) { + (true, Value::Int(value)) => Ok(Property { id: 0x06, - value: time(value)?, + value: time(*value)?, }), - _ => Err(Error::MalformedTrack), + (true, Value::Bytes(_)) => Err(Error::MalformedTrack), + (false, _) => Ok(property.clone()), }) .collect::, _>>()?; let mut sorted: Vec<_> = properties @@ -182,6 +214,18 @@ pub(super) fn write(properties: &Properties, version: Version, timescale: Option Ok(out) } +/// Whether the immutable block holds a property with `id`. +fn sealed_key(properties: &Properties, version: Version, id: u64) -> Result { + let Some(sealed) = properties.values.iter().find(|property| property.id == 0x0b) else { + return Ok(false); + }; + let Value::Bytes(bytes) = &sealed.value else { + return Err(Error::MalformedTrack); + }; + let nested = list(bytes, version, true)?; + Ok(nested.properties.values.iter().any(|property| property.id == id)) +} + #[cfg(test)] mod tests { use super::*; @@ -251,27 +295,61 @@ mod tests { /// in the units the egress declares, not the ones it arrived in. #[test] fn mutable_timestamps_follow_the_egress_units() { - for key in [0x10, 0x06] { + for (key, version) in [(0x10, Version::Draft22), (0x06, Version::Draft18)] { let bytes = Bytes::copy_from_slice(&[key, 42, 0x20, 7]); - let decoded = read(&bytes, Version::Draft22, Some(Timescale::MILLI)).unwrap(); - let written = write(&decoded.properties, Version::Draft22, Some(Timescale::MICRO)).unwrap(); - let again = read(&Bytes::from(written), Version::Draft22, Some(Timescale::MICRO)).unwrap(); + let decoded = read(&bytes, version, Some(Timescale::MILLI)).unwrap(); + let written = write(&decoded.properties, version, Some(Timescale::MICRO)).unwrap(); + let again = read(&Bytes::from(written), version, Some(Timescale::MICRO)).unwrap(); let micros = Timestamp::new(42_000, Timescale::MICRO).unwrap(); assert_eq!(again.timestamp, Some(micros), "key {key:#x}"); assert_eq!(again.properties.values.last().unwrap().value, Value::Int(7)); // An egress that declares no units drops it, as a plain frame there has none. - let written = write(&decoded.properties, Version::Draft22, None).unwrap(); - let again = read(&Bytes::from(written), Version::Draft22, None).unwrap(); + let written = write(&decoded.properties, version, None).unwrap(); + let again = read(&Bytes::from(written), version, None).unwrap(); assert_eq!(again.properties.timestamp, None, "key {key:#x}"); assert_eq!(again.properties.values.len(), 1, "key {key:#x}"); assert_eq!(again.properties.values[0].value, Value::Int(7)); // An untimed ingress forwards it untouched. - let untimed = read(&bytes, Version::Draft22, None).unwrap(); - assert_eq!(write(&untimed.properties, Version::Draft22, None).unwrap(), bytes); + let untimed = read(&bytes, version, None).unwrap(); + assert_eq!(write(&untimed.properties, version, None).unwrap(), bytes); } } + /// Draft-19 reassigns 0x06 from the draft-03 Timestamp to SUBGROUP_DELIVERY_TIMEOUT, + /// which keeps its value whatever the track's units. + #[test] + fn key_six_follows_its_draft() { + let bytes = Bytes::from_static(&[0x06, 42, 0x1a, 7]); + let milli = Some(Timescale::MILLI); + let decoded = read(&bytes, Version::Draft22, milli).unwrap(); + assert_eq!(decoded.timestamp, None); + for timescale in [Some(Timescale::MICRO), None] { + assert_eq!(write(&decoded.properties, Version::Draft19, timescale).unwrap(), bytes); + } + + // Across draft-19 neither reading survives: a mutable one is dropped. + let modern = Bytes::from_static(&[0x20, 7]); + assert_eq!(write(&decoded.properties, Version::Draft18, milli).unwrap(), modern); + let legacy = read(&bytes, Version::Draft18, milli).unwrap(); + assert_eq!(write(&legacy.properties, Version::Draft19, milli).unwrap(), modern); + + // A sealed one is refused. + let sealed = Bytes::from_static(&[0x0b, 2, 0x06, 42]); + let decoded = read(&sealed, Version::Draft22, milli).unwrap(); + assert!(!decoded.properties.sealed_time); + assert_eq!(write(&decoded.properties, Version::Draft22, None).unwrap(), sealed); + assert!(matches!( + write(&decoded.properties, Version::Draft18, milli), + Err(Error::Unsupported) + )); + let legacy = read(&sealed, Version::Draft18, milli).unwrap(); + assert!(matches!( + write(&legacy.properties, Version::Draft19, milli), + Err(Error::Unsupported) + )); + } + #[test] fn immutable_lists_validate_nested_blocks_and_integer_forms() { for bytes in [&[0x0b, 2, 0x0b, 0][..], &[0x0b, 0, 0, 0][..], &[0x0b, 1, 2][..]] { diff --git a/rs/moq-net/src/ietf/publisher.rs b/rs/moq-net/src/ietf/publisher.rs index f6b862e9faa..f4589c1a9e3 100644 --- a/rs/moq-net/src/ietf/publisher.rs +++ b/rs/moq-net/src/ietf/publisher.rs @@ -1787,6 +1787,7 @@ where if !matches!(self.version, Version::Draft14 | Version::Draft15) { group.end_object = None; } + group.end_object = group.end_object.map(|end| status_fields(end, self.version)); // FETCH keeps each object's Timestamp, in the units FETCH_OK declares wherever the // draft can declare them. Opting out of the properties strips nothing from the objects. let timescale = group @@ -3844,7 +3845,8 @@ impl GroupServe { } Poll::Ready(Ok(None)) => { if !self.ended - && let Some(end) = self.group.end_object() + && let Some(end) = + self.group.end_object().map(|end| status_fields(end, self.version)) && end.id >= self.slice.skip && self .slice .until @@ -3974,6 +3976,20 @@ fn object_delta(prior: Option, id: u64) -> Result { } } +/// A status object's fields, without properties where its draft forbids them. +fn status_fields( + end: std::sync::Arc, + version: Version, +) -> std::sync::Arc { + match super::object::status_has_properties(version) || end.properties.is_empty() { + true => end, + false => std::sync::Arc::new(crate::model::object::Fields { + id: end.id, + properties: Default::default(), + }), + } +} + fn buffer_object( writer: &mut Writer, object: Object<'_>, diff --git a/rs/moq-net/src/ietf/subscriber.rs b/rs/moq-net/src/ietf/subscriber.rs index ac3c0546678..4af60b0fe65 100644 --- a/rs/moq-net/src/ietf/subscriber.rs +++ b/rs/moq-net/src/ietf/subscriber.rs @@ -3827,7 +3827,11 @@ where } let size = stream.varint().await?; if size == 0 && matches!(self.version, Version::Draft14 | Version::Draft15) { - match stream.varint().await? { + let status = stream.varint().await?; + if status != 0 { + super::object::check_status(self.version, &decoded.properties)?; + } + match status { 0 => {} END_OF_GROUP => { if keep { @@ -4648,7 +4652,10 @@ impl GroupIngest { frame.finish()?; } self.phase = IngestPhase::Delta; - } else if status == END_OF_GROUP { + continue; + } + super::object::check_status(self.version, &self.properties)?; + if status == END_OF_GROUP { group.mark_gap(self.prior_object.expect("properties follow Object ID"), self.gap)?; group.set_end_object(crate::model::object::Fields { id: self.prior_object.expect("status follows object ID"), @@ -8793,11 +8800,21 @@ mod stitch_tests { /// relay fills its cache from that answer. Later drafts end the answer at the last frame. #[moq_net_sim::test] async fn passthrough_fetch_keeps_the_end_of_group_object() { - for version in [Version::Draft15, Version::Draft16, Version::Draft17, Version::Draft22] { + for version in [ + Version::Draft14, + Version::Draft15, + Version::Draft16, + Version::Draft17, + Version::Draft22, + ] { + // Only draft-14 lets a status object carry properties, so later drafts cannot mark + // a gap before it. + let legacy = version == Version::Draft14; + let terminal = if legacy { (11, &[0x3e, 2][..]) } else { (9, &[][..]) }; let objects = [ (6, &[0x3e, 6][..], None), (8, &[0x3e, 1][..], None), - (11, &[0x3e, 2][..], Some(END_OF_GROUP)), + (terminal.0, terminal.1, Some(END_OF_GROUP)), ]; let script = object_stream(version, true, &objects); let (bytes, log) = relay_passthrough(script, version, version, Some((0, 12))).await; @@ -8805,9 +8822,9 @@ mod stitch_tests { let mut decoder = Decoder::new(&bytes, version.into()); assert_eq!(decoder.varint().unwrap(), ietf::FetchOk::ID, "{version}"); let ok = ietf::FetchOk::decode(&mut decoder, version).unwrap(); - let carried = version == Version::Draft15; + let carried = matches!(version, Version::Draft14 | Version::Draft15); let last = match () { - _ if carried => 12, + _ if carried => terminal.0 + 1, _ if Filter::is_draft20(version) => 8, _ => 9, }; @@ -8836,13 +8853,57 @@ mod stitch_tests { let end_object = reader.end_object(); assert_eq!( end_object.as_ref().map(|end| end.id), - carried.then_some(11), + carried.then_some(terminal.0), "{version}" ); if let Some(end) = end_object { - assert_eq!(end.properties.values[0].id, 0x3e); + assert_eq!( + end.properties.values.first().map(|property| property.id), + legacy.then_some(0x3e) + ); + } + } + } + + /// Draft-15 on, only a Normal object may carry properties: a status object with them is + /// refused on ingest, and one relayed from draft-14 leaves without them. + #[moq_net_sim::test] + async fn passthrough_keeps_properties_off_status_objects() { + for version in [Version::Draft15, Version::Draft22] { + for status in [END_OF_GROUP, END_OF_TRACK] { + let script = object_stream(version, true, &[(0, &[], None), (1, &[0x20, 7], Some(status))]); + let mut h = Harness::with_timescale(Fill::Done, vec![script], None); + h.subscriber.version = version; + let mut stream = h.stream().await.with_version(version); + let result = h.subscriber.clone().recv_group(&mut stream).await; + assert!( + matches!(result, Err(Error::Session(SessionError::ProtocolViolation))), + "{version} {status}: {result:?}" + ); } } + + let script = object_stream( + Version::Draft14, + true, + &[(0, &[], None), (1, &[0x20, 7], Some(END_OF_GROUP))], + ); + let (bytes, log) = relay_passthrough_stream(script, Version::Draft14, Version::Draft15).await; + assert!(log.resets().is_empty()); + let mut decoder = Decoder::new(&bytes, Version::Draft15.into()); + ietf::GroupHeader::decode(&mut decoder, Version::Draft15).unwrap(); + assert_eq!(decoder.varint().unwrap(), 0); + decoder.bytes().unwrap(); + assert_eq!(decoder.varint().unwrap(), 1); + assert_eq!(decoder.slice(1).unwrap(), &[99]); + assert_eq!(decoder.varint().unwrap(), 0); + assert!( + decoder.bytes().unwrap().is_empty(), + "the status object kept its properties" + ); + assert_eq!(decoder.varint().unwrap(), 0); + assert_eq!(decoder.varint().unwrap(), END_OF_GROUP); + assert!(decoder.is_empty()); } #[moq_net_sim::test] diff --git a/rs/moq-net/src/model/object.rs b/rs/moq-net/src/model/object.rs index 4ddc0218c00..24e5ececce9 100644 --- a/rs/moq-net/src/model/object.rs +++ b/rs/moq-net/src/model/object.rs @@ -34,6 +34,9 @@ pub(crate) struct Properties { pub timescale: Option, /// The immutable block holds a Timestamp, which keeps its ingress units. pub sealed_time: bool, + /// The ingress draft predates 19, so 0x06 is a Timestamp rather than + /// SUBGROUP_DELIVERY_TIMEOUT. + pub legacy_time: bool, } impl Properties { @@ -42,6 +45,11 @@ impl Properties { self.values.is_empty() } + #[cfg(feature = "async")] + pub fn is_empty(&self) -> bool { + self.timestamp.is_none() && self.values.is_empty() + } + pub fn heap(&self) -> u64 { (self.values.capacity() * size_of::()) as u64 + self