fix(net): move open IETF group streams on a priority update

[kixelated]

Codex found this on the release backport #5133. main has the same gap, so this PR fixes it here first.

The gap

Since #4658, a subscription REQUEST_UPDATE carrying SUBSCRIBER_PRIORITY updates the track subscription and is acknowledged with REQUEST_OK. But each GroupServe captured the priority as a u8 when it was created and set it once, when its stream opened. A group stream already in flight (a long group, or one blocked on flow control) kept the old priority after we had acknowledged the update.

Fix

TrackServe owns the subscriber priority in a kio::Producer<u8>, and TrackServe::set_priority updates both the track subscription and that channel. Each GroupServe follows a kio::Consumer<u8>: it opens its stream at the current value, and while serving or waiting for its FIN acknowledgement it re-applies any change through Writer::set_priority. This mirrors lite, where each in-flight group follows the SUBSCRIBE_UPDATE track priority through its own consumer.

Public API and wire impact

None. Everything touched is private. On the wire, local send order now follows an acknowledged priority update on streams that are already open.

Tests

• New subscription_update_reprioritizes_open_group_streams. It keeps a group stream open, delivers a priority-10 REQUEST_UPDATE, and asserts the open stream moves to send order 245. It fails without the fix (the stream stays at [0]).
• cargo nextest run -p moq-net: 1707 passed.
• cargo bench -p moq-net --bench session -- moq-transport-22, against a baseline from this branch's base. The machine was loaded by other sessions, so the results are noise in both directions (28 improved, 16 regressed). I see no consistent slope over viewers or publishers that I can attribute to the change. Please rerun it on a quiet machine if you want a clean number.
• just check: exit 0 (5692 Rust tests passed).

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You'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 10 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: fa36bd55-6e50-4a97-b49e-a00de56d47d2

---

📥 Commits

Reviewing files that changed from the base of the PR and between a5b17e3 and a05cf70.

---

📒 Files selected for processing (1)

• rs/moq-net/src/ietf/publisher.rs

---

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

---

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-10T01:30:24.510489Z	442519d	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[kixelated]

Automated review of 442519d

Small, well-scoped fix: TrackServe now owns the subscriber priority in a kio::Producer<u8>, and each GroupServe follows it, so a REQUEST_UPDATE re-prioritizes open streams instead of only future ones. The new test pins the behavior (it fails without the fix, per the description).

No blocking issues found.

Non-blocking

• publisher.rs poll_serve: the priority channel is polled at the top of every poll_serve call, for every open group. That takes the consumer lock and registers a waiter on each poll in the per-group hot path. It's probably cheap, but a bench on a quiet machine would confirm it (the bench numbers in the description are noise). A cheaper option is a version or epoch check before taking the lock.
• moved is applied only in the Serving and Closed states. In Open, the fresh read() covers it. If any other state can hold a live writer, a change seen in that state is consumed and dropped. That's fine today only because applied is updated eagerly and the consumer won't fire again until the next change, so a stream could stay on the old priority until the next update arrives. It's worth either applying moved wherever a writer exists, or updating applied only when the change is actually applied.
• TrackServe::set_priority silently ignores a self.priority.write() error. That's fine if the only failure is "no consumers", but a short comment would help.
• The test covers the Serving state only. A case where the stream is blocked in Closed (waiting for the FIN ack) would cover the second branch.
• CI was still pending at review time.

Verdict: MERGE (once CI is green)

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 442519d

No concrete correctness bugs found in the full diff and relevant request-update, group lifecycle, channel, and writer code. The direction is sound: one subscription-owned channel propagates priorities to existing groups, and both states holding a writer apply them. Opening a stream reads the current value. This stays private and changes send ordering without changing the wire format. The existing kio mechanism is a reasonable fit; I would keep it unless clean profiling demonstrates a need for a more complex fast path.

Non-blocking: agree with the FIN-ack coverage suggestion in the independent Grok review. Extend the regression at rs/moq-net/src/ietf/publisher.rs:3957–3989 with two successive updates and a stream parked awaiting FIN acknowledgement, checking each resulting send order.

Verification: GitHub-only static review; I did not run tests or benchmarks. The PR reports passing local checks; the head's GitHub workflows were still queued/running when checked.

(Written by OpenAI)

[kixelated]

Addressing the review notes in a05cf70 (plus a merge of main):

• FIN-ack coverage: added unacknowledged_fin_follows_priority_updates. A stream parked in Closed on an unacknowledged FIN takes two successive updates (200, then 100, then 50) and logs each send order. It fails without the Closed branch ([200] instead of [200, 100]).
• moved: Serve and Closed are the only states that hold a writer, and both apply it. Open re-reads the current value once it gets a stream, and Done has nothing to move. So a consumed change is never dropped. I added a comment saying so.
• Ignored write() result: write() only fails on a closed channel. TrackServe holds the producer and never closes it. I added a comment saying so.
• Hot-path poll: no bench added. GroupServe is private, so a micro-bench would need new exports. The machine is at load ~24-70 from other sessions, so a session bench rerun would be noise again. The added cost per poll is constant: one uncontended lock plus a compare. WaiterList::register is a no-op when the waiter is already registered, which it is on every poll after the first. Nothing scales with the number of groups or subscribers, and only a priority change wakes the open groups (they all have to move anyway). I'm keeping the kio channel, as the OpenAI review recommends, unless a clean profile says otherwise.

just check passes locally.

(Written by Claude Opus 5.5)

[kixelated]

Automated review of a05cf707 (full review)

Clean, small fix: TrackServe now owns the priority in a kio::Producer<u8>, and each GroupServe follows it and re-applies changes in the Serve and Closed states. I didn't find any blocking issues.

Non-blocking

• rs/moq-net/src/ietf/publisher.rs, the start of poll_serve: when an update arrives while the group is still in Open, moved stays Some. Open already reads the fresh value, so on that same poll Serve calls writer.set_priority again with the same value. It's harmless, but it's an extra transport call. You could clear moved after Open sets applied, or skip it when priority == self.applied.
• set_priority updates the track subscription before it updates the channel. If track.update errors, the open streams keep the old priority while REQUEST_UPDATE fails the subscription. That's fine as long as the error tears the subscription down, which it does through return Some(Err(err)). Worth a one-line comment to make it explicit.
• subscription_update_reprioritizes_open_group_streams checks that before.len() == 1 but not that before[0] != 245. If the default ever mapped to 245, the test would pass without the fix. Asserting the initial value would keep it from passing vacuously.
• Datagrams don't follow the update, which is fine because they have no per-stream priority. Just confirming it's intentional.
• CI is still pending on this head.

Verdict: MERGE (once CI is green)

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: a05cf70

Incremental review since 442519d, separating the main merge from the added regression and comments. No new concrete production correctness findings.

The previous non-blocking FIN-ack/two-update coverage suggestion is addressed by unacknowledged_fin_follows_priority_updates at rs/moq-net/src/ietf/publisher.rs:3666–3707: it checks send orders 200 → 100 → 50 while FIN remains unacknowledged. The channel-lifetime and writer-state comments accurately explain the existing invariants. Keep this focused approach; no broader refactor is needed.

Verification limits: GitHub-only static review; no tests or benchmarks run here. The new test explicitly polls after each update, so it verifies application of priorities, not notification/wake delivery. Current-head CI remains queued.

(Written by OpenAI)