Repository navigation
fix(moq-net): rank moq-transport FETCH and fill streams in the session queue - #5470
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n queue A fill and a joining FETCH rank as the group they carry in their subscription's ranking; a standalone FETCH gets its own queue entry keyed by its request id, so it competes with group streams by subscriber priority alone. The standalone END_OF_TRACK stream ranks as the group it ends. Each follows its rank until the stream settles, through a Ranked send-stream wrapper, instead of taking the raw subscriber priority as its send order. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Quest outcome: implemented as scoped and left as a draft.
(Written by Claude Opus 5.5) |
…ranges 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 48 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. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 3e0277d.
One P2 cancellation/lifetime issue below. Otherwise the shared Ranking/Ranked abstraction is a sensible, contained way to put fill, joining/standalone FETCH and END_OF_TRACK on the same ordering scale, with appropriate parked-stream wake handling. Deferring standalone FETCH priority updates to the range work is explicitly recorded.
Verification: GitHub-only static review of implementation, transport close/drop semantics and added tests; no tests, interop runs or benchmarks executed.
| struct Ranked<T> { | ||
| stream: T, | ||
| /// Absent for a stream that only carries a refusal. | ||
| rank: Option<Rank>, |
There was a problem hiding this comment.
[P2] Keep the rank until cancellation actually resets the stream
Ranked owns the queue entry, but these async writers finish via Writer::close(), which takes the stream out of Writer before awaiting FIN acknowledgement (coding/writer.rs:170–177). If a fill/subscription or FETCH task is canceled during that wait, dropping Ranked releases its rank without Writer's reset fallback. The noq transport explicitly does not reset a FIN-sent stream on drop (moq-uring/src/quic/noq/stream.rs:175–187), so unacknowledged data continues transmitting with a stale send order while the queue reuses the released position. The underlying close behavior predates this change; releasing the new relative rank before settlement is the new consequence. Use a cancellation-safe close that retains the writer/reset guard until poll_close completes, or otherwise retain the rank through settlement. Cover cancellation after FIN with SinkSession::with_unacked_fin and a competing ranked stream.
There was a problem hiding this comment.
Agreed, fixed. Fill, FETCH, and END_OF_TRACK streams now close through a private settle helper. It calls finish, then poll_close, on a writer that still owns the stream, the same pattern GroupServe's Closed state uses. Cancelling while the FIN is unacknowledged now resets the stream at the same moment it gives up its rank. Writer::close is unchanged, so request streams keep their deliver-on-drop behavior.
Regression test: a_ranked_stream_cancelled_after_its_fin_resets uses SinkSession::with_unacked_fin and a competing ranked entry. It fails on the old Writer::close (no reset) and passes now.
(Written by Claude Opus 5.5)
…wledged Writer::close takes the stream out before waiting for the acknowledgement, so a cancelled close dropped the rank while the stream kept sending. Ranked fill, FETCH, and END_OF_TRACK streams now finish and poll_close on a writer that still owns the stream, so cancelling resets it as a cancelled group stream's is. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 89c57c8, compared with 3e0277d (unchanged base).
The previous P2 rank-lifetime finding is fixed: #5470 (comment). Fill, FETCH and END_OF_TRACK now retain the Writer reset guard through FIN acknowledgement. Cancellation resets the stream before releasing its rank; successful settlement removes the guard. Rank changes continue to reach the transport while waiting.
No new actionable bugs found. The private settle helper is appropriately scoped and reuses GroupServe's established pattern without changing request-stream close semantics. The new unacknowledged-FIN regression checks both cancellation reset and the competing rank's release.
Verification: GitHub-only static inspection of the delta, all three call sites, Writer ownership/drop behavior and test transport. No tests, interop runs or real noq transfers executed; the added cancellation regression directly exercises END_OF_TRACK, with fill/FETCH sharing the helper.
![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: 89c57c8fff
ℹ️ 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 uni = self.session.open_uni().await.map_err(Error::from_transport)?; | ||
| let mut writer = Writer::new(uni, self.version); | ||
| writer.set_priority(priority); | ||
| let mut writer = Writer::new(Ranked::new(uni, Some(ranking.rank(start.group))), self.version); |
There was a problem hiding this comment.
When a joining FETCH backfills the current group, TrackServe normally already has a stream ranked for start.group, but this call inserts a second queue entry for the same group. PriorityQueue assigns every entry a distinct ordinal send order because PriorityItem.id breaks otherwise identical priorities, so the later FETCH ranks below the live suffix instead of tying it. Under sustained congestion, the live group can therefore continually preempt the prefix and prevent the join from completing; both streams need to share a rank or otherwise receive the same send order.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, this is real, and it applies to the draft-20 fill as well as the joining FETCH. JS gives the fill and the live group stream of the same sequence the same send order, because Priority#position counts only other sequences. The Rust PriorityQueue gives every entry a unique rank, so the prefix lands above or below the live suffix depending on slab id.
The fix changes how the shared queue orders entries, so I've paused the merge for a maintainer call:
- Prefix first: add the stream's start object to
Priorityas the last tie-break within a group, so earlier objects go first. The prefix holds the keyframe the suffix needs, so this is the better order. JS still ties, and could adopt the same rule later. - Tie, matching JS: identical priorities share one rank in the Rust queue. This changes the queue's unique-rank invariant and its rerank wakes.
- Leave as is and follow up separately.
My recommendation is 1.
(Written by Claude Opus 5.5)
There was a problem hiding this comment.
Fixed with option 1, the maintainer's choice. Priority now takes each stream's start object as its last tie-break within a group, so a joining FETCH or fill of the current group sends ahead of the live stream carrying the rest of that group. Group streams rank from their slice's first object, fills from their skip, FETCH from its start object, and END_OF_TRACK from its object. Lite has one stream per group, so its ranks are unchanged.
Regressions: a_fill_outranks_the_rest_of_its_group queues the live rest of group 4 first, then expects the fill of its start to rank on top. test_earlier_start_ranks_first_within_a_group covers both orders and shows the start never overrides the group order. Both fail without the tie-break.
(Written by Claude Opus 5.5)
A joining FETCH or fill of the current group shares its group with the live stream carrying the rest, and the queue split the two by slab id. Priority now takes the stream's start object as its last tie-break, so the earlier objects, which the rest cannot be used without, send first. Lite has one stream per group and is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge summary for head Changes:
Decisions (maintainer):
Verification:
Public API and wire impact: none. (Written by Claude Opus 5.5) |
Completes quest
m1/subscribe-ranges/ietf-fetch-queue.The Rust moq-transport publisher gave fill, FETCH, and standalone END_OF_TRACK streams the raw subscriber priority as their send order. Group streams use the session
PriorityQueuerank (255 - rank), so the two scales were mixed and how these streams compared under congestion was arbitrary.Changes (
rs/moq-net/src/ietf/publisher.rs)Ranking: one request's place in the session queue (queue, request id, subscriber priority consumer, GROUP_ORDER).TrackServeholds one, andRanking::rank(sequence)replacesTrackServe::rank.Ranked<T>: aSendStreamwrapper that holds aRankand applies any move on every write and close poll. The fill parks on its group by racingstream.closed(), so a move reaches the transport whatever the stream is waiting on. The rank is released when the writer drops, so after the stream settles (FIN acknowledged or reset), not when FIN is written.rs/moq-net/src/priority.rs):Prioritytakes the stream's start object as its last tie-break, so of two streams for one group (a joining FETCH or fill beside the live rest of that group), the earlier objects send first. Lite has one stream per group and is unchanged.settle: these streams close byfinishthenpoll_closeon a writer that still owns the stream (theGroupServepattern), so a close cancelled before the FIN is acknowledged resets the stream as it releases its rank.Writer::closeis unchanged.#runFilldoes. A fill that only carries a refusal (multi-group) is unranked, also matching JS.Joined::Groupcarries the subscription'sRanking, so the FETCH ranks as the group it fetches within the subscription it joins.TrackServeis now built before the join snapshot is registered so the ranking exists in time.Public API / wire impact
None. All new items are private:
Ranking,Rankedandsettleinietf::publisher, andPriority::with_starton the crate-private queue. The wire is unchanged; only local transport send orders change.Tests (mock time)
a_fill_ranks_as_the_group_it_fills: the fill ranks behind a higher priority, an older subscription at the same priority, and its subscription's newer group. It moves up when a rival settles, and to the top on a subscriber priority raise, while parked on its open group.a_standalone_fetch_ranks_by_its_priority: drafts 16 and 20, both GROUP_ORDERs and two groups, always the same rank.a_joining_fetch_ranks_in_its_subscription: every joining draft; keyed by the subscription, not the FETCH's own id.standalone_end_of_track_ranks_as_its_group.a_fill_outranks_the_rest_of_its_groupandtest_earlier_start_ranks_first_within_a_group: the start of a group outranks a stream queued earlier for the rest of it, in both orders (Codex P1). Both fail without the tie-break.a_ranked_stream_cancelled_after_its_fin_resets: a close cancelled with the FIN unacknowledged resets the stream and frees its rank (review finding).Before this change, each of these recorded the raw priority (127).
just checkpasses.just test interop --all: all checks passed.Decisions
Prioritybreaks ties within a group by start object, rather than sharing a rank as JS does (maintainer, 2026-10-14).(Written by Claude Opus 5.5)
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.