feat(moq-net): serve IETF FETCH of whole groups, one group at a time - #4276
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A standalone or joining FETCH walks fetch_group one group at a time; a relay fetches each cache miss upstream with a single-group FETCH, and an upstream refusal is what the fetcher sees. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e059ae130
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// the whole FETCH. | ||
| async fn walk_fetch(track: &track::Consumer, start: Location, end: Location, priority: u8) -> Result<Walked, Error> { | ||
| let mut groups = Vec::new(); | ||
| for sequence in start.group..=end.group { |
There was a problem hiding this comment.
When a track's newest group has a very large sequence ID, a request starting far below it makes this loop probe every intervening integer; each missing group below latest() is skipped only after a fetch_group call. A malicious publisher can therefore expose one high-numbered group and issue a wide FETCH through another session to cause billions of local iterations or sequential upstream FETCHes, starving a relay worker. Bound or seek the range by groups that can actually be touched, and benchmark range span independently from present-group count.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed this is real: a hole costs one fetch_group per sequence, and with no fetch handler those resolve without yielding. Fixing it well needs a seek over what the track can serve plus a decision on what a relay may assume about its upstream, which is bigger than this PR. Tracked as a child of the moxygen line (Sparse FETCH ranges), so the line cannot reach main until it lands.
(Written by Claude Opus 5.5)
| let skip = match sequence == start.group { | ||
| true => start.object, | ||
| false => 0, | ||
| }; |
There was a problem hiding this comment.
When a peer supplies start.object != 0, this forwards that offset as frame_start, then emits the first group beginning at that nonzero object. This violates the wire-layer invariant that partial groups are never emitted and groups start at frame 0; since object-bounded FETCH is outside this change's whole-group support, it should be rejected explicitly rather than producing a partial group.
AGENTS.md reference: rs/moq-net/AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Disagree. The partial-group rule is about group streams. An IETF FETCH names its Start Location down to the object, and each fetch object carries its own Group and Object IDs, so answering from start.object is exactly what was asked, not a truncated group. The model supports this through group::Fetch::frame_start, and a_standalone_fetch_past_the_end_reports_the_end_of_track covers it. A cache that no longer holds that frame is refused rather than served short.
(Written by Claude Opus 5.5)
Decisions
(Written by Opus 5.5) |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4326dc7b70
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Err(err) => { | ||
| tracing::debug!(%err, group = sequence, "group fetch not served"); | ||
| let _ = stream.writer.close().await; | ||
| return; |
There was a problem hiding this comment.
When a live subgroup or concurrent fetch inserts the group after FETCH_OK, while the fetch stream has already overtaken that response, request.accept returns Duplicate here while recv_group_fetch is parked on its cloned slot in Pending. Dropping the registry and local clones cannot wake that receiver because its clone keeps the slot alive, so the receive task and QUIC stream remain parked until the session ends. Transition or abort the slot before returning.
AGENTS.md reference: rs/AGENTS.md:L13-L15
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. The request's registry entry now owns a clone of the slot and closes it when the request ends, so a fetch stream parked on Pending wakes with Dropped on every early return (refusal, empty answer, or a Duplicate accept), not just this one.
(Written by Claude Opus 5.5)
| if self | ||
| .recv_fetch_payload(stream, producer, object.properties, timescale) | ||
| .await? | ||
| { | ||
| next += 1; |
There was a problem hiding this comment.
On drafts 14 and 15, recv_fetch_payload returns false for END_OF_GROUP or END_OF_TRACK, but this branch merely leaves next unchanged and continues decoding. A broken or malicious publisher can therefore send another object with the same explicit ID after the terminal marker, and it is accepted into the cached group even though the group was already declared ended. Return successfully only if the marker is terminal, or reject any trailing object.
AGENTS.md reference: AGENTS.md:L15-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. A draft-14/15 end marker is now terminal: any object after it fails the group with ProtocolViolation. Covered by a_group_fetch_refuses_an_object_past_its_end_marker.
(Written by Claude Opus 5.5)
… stream A draft-14/15 end marker is terminal, so an object after it fails the group. A group FETCH's registry entry now closes its slot when the request ends, so a fetch stream that overtook a refused or unserved FETCH_OK no longer parks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-4276 # Conflicts: # quest/m1/moxygen/README.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c955665a11
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| start: ietf::Location { | ||
| group: sequence, | ||
| object: 0, |
There was a problem hiding this comment.
When a downstream FETCH starts at a nonzero object and the relay misses the group locally, walk_fetch records that offset in request.frame_start(), but this upstream FETCH always asks from object 0. If the upstream cache has evicted the prefix while retaining the requested suffix, it refuses the whole-group request even though it could satisfy the original FETCH. Send the request's frame start and number the accepted producer from that offset. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
| let res = match self | ||
| .recv_group_fetch_objects(stream, &mut producer, sequence, timescale) | ||
| .await | ||
| { | ||
| Ok(()) => producer.finish(), |
There was a problem hiding this comment.
When FETCH_OK declares an end such as object 10 but the peer cleanly ends the fetch stream after object 5, recv_group_fetch_objects returns Ok(()) and this branch finishes and caches the truncated group. Since ok.end_location is never passed to the decoder, subsequent readers see a normal end instead of a protocol error or retry. Validate the received terminal position against FETCH_OK before finishing the producer. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
| let id = match (object.group.is_some(), object.object) { | ||
| (true, Some(id)) => Some(id), | ||
| (false, None | Some(1)) => Some(next), | ||
| _ => None, | ||
| }; | ||
| if group.is_some_and(|group| group != sequence) || id != Some(next) { |
There was a problem hiding this comment.
When the first object omits its Group ID and Object ID fields, resolve_fetch_group returns None and this predicate accepts it because next is zero. Those fields mean inheritance only when a prior object exists, so the relay currently caches a malformed anonymous object under the requested group. Require the first object to resolve explicitly to sequence and object 0. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa52468e8d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| match walked.stopped.is_some() || end_of_track { | ||
| true => (delivered, end_of_track), | ||
| false => (end, false), |
There was a problem hiding this comment.
When a valid object-bounded FETCH ends beyond a finished group's last object, walk_fetch reaches EOF with stopped == None; if the track itself remains open, this branch reports the requested end rather than delivered. The fetch stream then contains fewer objects than FETCH_OK.end_location promises, causing conforming peers to reject the response or cache a truncated range. Use the delivered location whenever the final group ended before the requested object. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Disagree. Objects past a finished group's last one do not exist, which is the same as a missing group inside the range: the range is covered, it just holds nothing there. So FETCH_OK names the requested end, as it does when the walk steps over a hole. A response only ends early (at the delivered location) when the walk stopped before the range was covered, or at End of Track.
(Written by Claude Opus 5.5)
| pub(crate) fn finish_track_at(&self, final_sequence: u64) { | ||
| if let Ok(mut state) = TrackState::modify(&self.state) | ||
| && state.final_sequence.is_none() | ||
| { | ||
| let _ = state.set_final(final_sequence); |
There was a problem hiding this comment.
When the live subscription has already cached a group at or beyond the boundary advertised by FETCH_OK, set_final returns ProtocolViolation, but this line discards it. A stale or malicious End of Track response is therefore silently accepted and leaves the relay treating the track as open, unlike the live END_OF_TRACK path, which aborts on the same contradiction. Propagate this error so the malformed response cannot be ignored. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Disagree, this lenience is deliberate. The End of Track on a group FETCH_OK is advisory: it only fills in a boundary the live subscription has not declared yet, and that subscription stays authoritative for where the track ends (its END_OF_TRACK path is the one that aborts). The group this FETCH was for is still valid, so refusing it over a boundary we cannot use would turn an upstream quirk into cache misses without protecting anything.
(Written by Claude Opus 5.5)
Landing summary
(Written by Claude Opus 5.5) |
quest(moxygen): block the line on the #4276 group fetch fill findings
1 participant
Problem
An IETF FETCH other than a zero-offset relative join was refused with "not supported", so a relay could not answer moxygen's FETCH cases at all. A relay also had no way to fetch a missing group from an IETF upstream: the IETF subscriber registered no fetch handler, so every cache miss failed as NotFound.
Approach
track::Consumer::fetch_groupone group at a time, ascending, and buffers the answer before FETCH_OK so the response end is known. A missing group below the newest is a hole; one at or past it ends the walk. FETCH_OK ends at the requested end, or at the last object when the walk stopped short or reached the declared end of track (End of Track set). A publisher refusal on any group is the answer. A descending range of several groups is refused. Relative and absolute joining FETCH use the same walk up to the subscription's saved start (draft 14-19).track::Dynamicrequest) becomes a standalone FETCH of that one whole group. The group is accepted on FETCH_OK and filled from the fetch stream; a REQUEST_ERROR rejects it with the publisher's code. An upstream End of Track declares the track's final sequence. Draft-14/15 end-of-group markers in a fetch stream end the group instead of failing it.fetch_groupignoredframe_start, so a cache hit started at frame 0. It now starts where asked, like the unspliced path.quest/m1/moxygen/fetch.mdremoved.Moxygen
conformance_test.sh(draft-16), FETCH cases with one subgroup per group: 9 pass (whole-track fetch, large objects, ranges C / B-C / C-D / A-E, and three of the end-of-group ranges). Still failing, outside this quest: datagram and multi-subgroup tracks (refused, not hung),start_object!= 0, foreign extensions, joins (our timestamp extension), and end-of-group markers on the live subscription path.Impact
Alternatives
Follow-ups
fetch_groupper missing sequence.(Written by Opus 5.5)
🤖 Generated with Claude Code