fix(net): follow-ups to #5133 (deferred FIN on drop, reprioritize open group streams)

[kixelated]

Follow-up to #5133, fixing the two Codex P2 findings on that PR. Both bugs also exist on main, which has its own PRs: #5156 and #5162.

1. A deferred FIN is lost when a send stream drops mid-watch (moq-tokio)

#5133 pulled in #4658's moq-tokio change, which lets poll_closed watch an idle, unfinished send stream. That watch (SendState::Closing) owns the stream. A finish() while it is held is only deferred, and Drop's Closing branch rescued a deferred reset but not a deferred FIN. Dropping the stream then gave the backend's default reset instead of the FIN that finish() had reported as successful. The Closing drop branch now applies a deferred finish too, like the Writing branch. This is a cherry-pick of #5156, and it applied cleanly.

moq-net's Writer resets on drop unless closed, so today's moq-net callers do not hit this path. It is still a contract bug in the adapter.

2. A priority REQUEST_UPDATE did not move group streams already in flight (moq-net)

#5133 applies a priority-only REQUEST_UPDATE to the track subscription and acknowledges it. But each GroupServe captured its priority once, so an open stream (a long group, or one blocked on flow control) kept the old priority. TrackServe now owns the priority in a kio channel that every group stream follows while open: it is applied at open, while serving, and while waiting for the FIN acknowledgement. Lite already does the same for SUBSCRIBE_UPDATE. This is a backport of #5162; I re-applied it by hand because the surrounding code differs on release.

I considered refusing priority updates instead. That would end a subscription over a benign update, and the fix is small, so I applied the update fully instead.

Public API and wire impact

None. Everything touched is private. On the wire, a FIN that was promised now goes out instead of a reset, and an acknowledged priority update now applies to open streams too.

Tests

• a_deferred_finish_survives_a_drop_mid_watch (moq-tokio, paused time): fails without fix 1.
• subscription_update_reprioritizes_open_group_streams (moq-net, paused time, draft-22): fails without fix 2 (the open stream stays at send order 127) and passes with it.
• unacknowledged_fin_follows_priority_updates (moq-net): a stream parked on its FIN acknowledgement follows two successive updates. Mirrored from fix(net): move open IETF group streams on a priority update #5162's review follow-ups, along with its comments.
• just check origin/release on 55089ca: exit 0 (the main Rust run: 5026 passed).
• cargo nextest run -p moq-net: 1522 passed. cargo nextest run -p moq-tokio: 432 passed.
• just check against origin/release: exit 0. A first run had moq-tokio::broadcast broadcast_race_quic_wins fail on AddrInUse while binding its server, a local port collision; the rerun passed everything.
• just test bare-fin: both tests pass.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[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:52:23.461067Z	6f0a2bd	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 6f0a2bd

Both fixes look right and each has a test that fails without it.

Non-blocking

• rs/moq-net/src/ietf/publisher.rs poll_serve: moved is taken once, before the state loop. If the stream goes from Open to Writing in the same poll, the update is applied twice, which is harmless. If an update lands while the group is still in Open, applied is reset from read() at open, so nothing is lost. A short comment saying the early poll registers the waker even in Done/Open would help the next reader.
• TrackServe::set_priority: a failed self.priority.write() is silently ignored after track.update succeeded, so the subscription and the open streams could disagree. That's probably unreachable while TrackServe owns the producer, but a debug_assert or a trace would make that explicit.
• Drop for SendState::Closing in moq-tokio: if both reset and finish are set, reset wins. That matches the Writing branch, but no test covers it.
• The test only covers the Writing state. A case for Closed (waiting on the FIN ack) would lock in the third application point.
• CI status wasn't checked in this pass.

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: 6f0a2bd

No concrete new correctness bugs found in the full diff and relevant request-update, group lifecycle, kio, writer, and transport code. The direction is sound: preserve the deferred FIN with the existing interrupted-watch salvage path, and propagate subscription priority through the existing kio mechanism. Keeping these fixes local is preferable to rejecting benign updates or introducing another scheduler abstraction.

Non-blocking coverage: agree with the independent Grok review. Extend rs/moq-net/src/ietf/publisher.rs:4623–4643 with two successive priority updates while a stream awaits its FIN acknowledgement; extend rs/moq-tokio/src/transport.rs:1184–1201 with both deferred finish and reset, asserting that only the reset wins.

Verification: GitHub-only static review; I did not run tests or benchmarks. The four head workflows (Check, WASM, Platform, Android) were queued when checked; the local test results are author-reported.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6f0a2bddad

ℹ️ 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".

[chatgpt-codex-connector]

Add a benchmark for priority-update fanout

Each open group now owns a consumer of the shared priority channel, so one REQUEST_UPDATE wakes and polls every in-flight group across affected subscriptions. The added test covers only one stream, and the existing rs/moq-net benchmarks do not exercise this IETF update path. Add a benchmark sweeping subscription and open-group counts so superlinear fanout costs are visible.

AGENTS.md reference: AGENTS.md:L33-L36

Useful? React with 👍 / 👎.

[kixelated]

Merge summary

#5162 merged on main (799d8c0). 55089ca mirrors its review follow-ups onto release:

• New test unacknowledged_fin_follows_priority_updates: a stream waiting on its FIN acknowledgement follows two successive priority updates.
• A comment in GroupServe::poll_serve: only the writer-holding states (Serve, Closed) apply a moved priority, and Open reads the current value when it gets its stream.
• A comment in TrackServe::set_priority: the ignored write fails only on a closed channel, and the producer never closes it.

Decisions, matching #5162:

• No fanout benchmark (the Codex P1 note). Main answered the hot-path note without one, so the backport follows it.
• No test for both a deferred reset and a deferred finish on the Closing drop. Reset wins there, as it does on the Writing branch.

The rest is review-only on top of 6f0a2bd, which the OpenAI and Codex reviews cover. just check origin/release passes locally. release has advanced (#5132, #5124), and the branch still merges cleanly. Lands as a merge commit, per CONTRIBUTING.md.

(Written by Claude Opus 5.5)

[kixelated]

Automated follow-up review of 55089ca (since 6f0a2bd)

This push is just tests and comments, and it covers three of the earlier notes:

• Fixed: there's now a Closed-state test (unacknowledged_fin_follows_priority_updates), which checks that a stream waiting on its FIN ack still applies each priority update (200, then 100, then 50).
• Fixed: poll_serve now has a comment saying which states apply the update and that Open reads the current value when it gets its stream.
• Fixed: set_priority now has a comment explaining why a failed priority.write() can't happen.

Still open, all non-blocking:

• There's still no test for SendState::Closing drop in moq-tokio when both reset and finish are set.
• CI is still pending on this head.

No new issues.

Verdict: MERGE (once CI is green)

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