Repository navigation
fix(net): join pre-draft-20 subscriptions at the live group - #6348
Conversation
A resumed subscription now carries each reader's resume floor as its start (#6209). Before draft 20 that start became an AbsoluteJoining FETCH from the floor through the live edge, which a publisher serving one group per FETCH refuses, and the refusal quietly continued live without the live group's head. Every pre-draft-20 join is now a live join: Largest Object plus a RelativeJoining FETCH for the Largest group alone, leaving earlier groups to group FETCHes as before #6209. A refused or failed joining FETCH now fails the subscription with its reason instead of leaving readers waiting on a head that never comes. The multi-group stitching only an absolute join needed is gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 9 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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. |
# Conflicts: # rs/moq-net/src/ietf/subscriber.rs
|
Merged Checks on 6f81650: (Written by Claude Opus 5.5) |
![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: 6f81650730
ℹ️ 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".
| let _ = stream.writer.close().await; | ||
| } | ||
|
|
||
| /// `true` when the publisher answered FETCH_OK. A FETCH_ERROR / REQUEST_ERROR is a | ||
| /// refusal, not a session error: the live subscription continues. | ||
| async fn read_fetch_response( | ||
| &self, | ||
| stream: &mut Stream<S, Version>, | ||
| messages: &mut super::request_stream::Messages<S>, | ||
| ) -> Result<bool, Error> { | ||
| let super::request_stream::Framed(type_id, body) = messages.message(&mut stream.reader).await?; | ||
| let mut data = body.decoder(self.version); | ||
|
|
||
| match type_id { | ||
| ietf::FetchOk::ID => { | ||
| let _msg = ietf::FetchOk::decode_msg(&mut data, self.version)?; | ||
| Ok(true) | ||
| } | ||
| ietf::FetchError::ID if self.version == Version::Draft14 => { | ||
| let _msg = ietf::FetchError::decode_msg(&mut data, self.version)?; | ||
| Ok(false) | ||
| } | ||
| ietf::RequestError::ID => { | ||
| let _msg = ietf::RequestError::decode_msg(&mut data, self.version)?; | ||
| Ok(false) | ||
| } | ||
| _ => Err(Error::UnexpectedMessage), | ||
| } | ||
| res |
There was a problem hiding this comment.
When the publisher returns FETCH_ERROR or REQUEST_ERROR but does not acknowledge our FIN, writer.close().await remains pending, so the newly captured res is never returned to JoiningTask and the subscription continues hanging instead of failing with the publisher's reason. Report the error before waiting for request-stream cleanup, or drive that cleanup independently; the repository's with_unacked_fin transport explicitly models this peer behavior.
AGENTS.md reference: AGENTS.md:L18-L19
Useful? React with 👍 / 👎.
Keeps main's ietf subscriber (#6348 joins pre-draft-20 subscriptions at the live group and refuses a multi-group joining fill) and serves track fills on top of it; signed priorities (#6142) reach Fill; the ledger moved to the crate root (#5878, #6111). The miss judge reads the live arrival grace rather than the ledger's stored one, which stays at its 1s default until a gap folds. Also answers review: a fetch of u64::MAX fails at once rather than parking on a handler no fill can ask, Subscription::start builds without `async`, and the MIGRATE guides no longer call GroupDemand or group::Request unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bug
moq-dev/moq#6209 made a resumed subscription carry each reader's resume floor as its start. Before draft 20,
subscribe_jointurned that start into anAbsoluteJoiningFETCH from the floor (say group 1) through the live edge. Our publisher serves one group per FETCH and refuses it ("FETCH spanning several groups not supported"). The floor group's leftover frames still arrive through the resume refetch, but the live group never does. The refusal was also swallowed: the subscriber "continued live" with no head for the live group, so a reader hung instead of failing. moq-dev/moq#6232'sroute_change::ietf_17_resumes_after_a_lagging_route_diespasses on its own head and hangs once main is merged in.Fix
RelativeJoiningFETCH at offset 0, so the FETCH carries only the Largest group's head. Earlier groups go to their own group FETCHes, as they did before fix(net): deduplicate resumed datagrams by identity #6209. The track's start is that Largest group, which is what the feed actually delivers.JoiningFetch,TrackState::{joining, largest},advance_fill_group,end_fill_group,Live/settle_join_live, and the datagram-group skipping. A fill that spans groups is now refused. Two FETCH-response readers are merged into one.Why this differs from the suggested
Absolute { group_id: max(start, largest) }: when start equals or is below Largest,RelativeJoining{0}means exactly the same thing on the wire, and the live join already uses it. When start is past Largest,AbsoluteJoining{start}is an invalid range that publishers refuse, and with fail-loud that refusal would kill the subscription. One spelling handles both cases. It still fetches the Largest group's head, which a merged live reader may want (subscribe_rangescovers this), and a reader starting later skips that head locally.Tests
a_join_from_any_start_fetches_only_the_largest_group: a start below, at, or past Largest sendsRelativeJoining{0}on every draft from 14 to 19. Fails on main, which sendsAbsoluteJoining{7}.a_refused_fetch_fails_the_subscription: a FETCH_ERROR/REQUEST_ERROR ends the track withUnsupportedand drops the subscription. Fails on main, which continued live. It replacesa_refused_fetch_continues_live.a_joining_fetch_spanning_several_groups_is_refusedreplaces the absolute-join stitching tests.older_drafts_always_join_livereplaces the per-startsubscribe_joincases.ietf_17_resumes_after_a_lagging_route_diespass. All 3374 moq-net and moq-relay tests pass there and on this branch, and clippy and fmt are clean.js/net sends no joining FETCH, so it needs no change.
API and wire impact
RelativeJoining{0}where it used to sendAbsoluteJoining{start}. This is valid on every draft, with no change to the format.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code