Repository navigation
fix(cli): moq play drops its Update workaround now that follow reports gaps - #5205
Conversation
…s gaps origin::Consumer::follow reports a gap onto a same-epoch covering prefix as an End then a Start (#5188), so playing an Update while idle is no longer needed. The idle covering-route test is the regression: it fails without #5188 once the workaround is gone. A new test plays through an exact-to-prefix handoff while the old run still holds its delay. 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. |
|
Automated review of Small, correct cleanup: with #5188 merged (moq-net Non-blocking:
Verdict: MERGE This is an automated review, not the maintainer's decision |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: d9f9bf6
No actionable bugs found in the full diff and relevant surrounding code. Direction is sound: rs/moq-cli/src/play/media.rs:81–86 now relies on the existing follow contract instead of treating an idle Update as a new start. I traced the current follower's same-epoch gap handling and the origin cursor's preserved retract/reannounce sequence; both retain the End/Start path that restarts playback.
The idle regression at media.rs:775–813 and handoff test at media.rs:820–860 cover complementary cases. As the PR and independent review note, the new test checks reaching the covering route with buffered playback; it does not by itself pin coalesced-gap reporting. The deterministic follow_keeps_a_gap_between_routes_of_one_epoch test covers that contract. No extra retry mechanism or broader refactor appears necessary.
Verification: GitHub-only source review and event tracing. I did not execute tests or verify end-to-end playback/CI results.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughMedia.play no longer starts playback from an update event when idle. The tests cover a same-epoch covering route announced after a gap and playback reaching audio from a covering route after the exact route is withdrawn. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to This change removes a workaround from the CLI player and adds tests for idle playback after a gap and for handoff to a covering route. No concrete merge-blocking risk was found. Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 1 files. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly and concisely identifies the main change: removing the moq play Update workaround. |
| Description check | |
The description directly explains the problem, implementation, tests, behavior impact, and relation to #5188. It is fully related to the changeset. |
✨ Finishing Touches
-
-
✨ Simplify code
-
-
- Commit to this branch
- Create a new PR
-
-
-
- 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.
Comment @coderabbitai help to get the list of available commands.
![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: d9f9bf6202
ℹ️ 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".
| // folded away (a covering route of one epoch arriving as the exact one | ||
| // went), so play it. | ||
| AnnounceEvent::Update(announce) if playing.is_none() => announce, | ||
| AnnounceEvent::Update(_) => continue, |
There was a problem hiding this comment.
When the initial Start races an advertise-only route or its handler, Source::broadcast() can fail with Unroutable, and the result branch leaves playing as None. The origin deliberately emits an Update when servability changes so failed requests can retry (rs/moq-net/src/model/origin.rs:1117-1119), but this unconditional continue discards that recovery signal, leaving playback blank until an unrelated restart occurs. Preserve recovery for an idle player or use the origin's routed resolution behavior for the initial subscription.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this. An advertise-only route (server: None, source: None) is only built by origin::Producer::announce, which is #[cfg(test)]. Every production route serves what it claims: broadcast::Producer::announce carries its source, and origin::Producer::dynamic (what both the lite and IETF subscribers use for peer announces) carries a server. So a Start never lands on a route that answers Unroutable for lack of a server, and the servability flip in origin.rs cannot strand a player outside tests.
Reviving an idle player on every Update would also retry on arbitrary re-pricing, which is the ambiguity this PR removes: an Update is the same instance, and a gap is End then Start since #5188.
(Written by Claude Opus 5.5)
|
Merge summary
Auto-merge armed on (Written by Claude Opus 5.5) |
|
Merging. Head Reviews of this head: the OpenAI review and CodeRabbit found nothing. Codex raised one P2, a retry on a servability-flip (Written by Claude Opus 5.5) |
1 participant
Problem
moq playstarted playback on an announceUpdatewhile nothing was playing. That branch existed only becauseorigin::Consumer::followused to fold a same-epoch handoff from the exact route to a covering prefix into anUpdate, hiding the gap that ended the old request. #5188 fixed that in moq-net:follownow reportsEndthenStart, which the player already handles.Approach
Drop the idle-
Updatebranch. EveryUpdateis again the same instance re-priced or failed over, which a playing run rides out and an idle player has nothing to do with.Tests:
an_update_while_idle_plays_the_routeis renameda_covering_route_after_a_gap_plays_while_idle, and its comment now describes the gap rather than anUpdate. It is the regression for this change: with the workaround removed, it fails onmainbefore fix(moq-net): a followed path reports a gap onto a same-epoch prefix #5188 and passes after it.a_handoff_to_a_covering_route_plays_on_while_draining. The exact route is unannounced while the old run still holds its 500 ms delay, and a same-epoch covering route serving a mirror of the same content takes over. Playback must reach the covering route. It passes with or without fix(moq-net): a followed path reports a gap onto a same-epoch prefix #5188 (with a local origin the player either seesEndthenStartor the origin rides the handoff out on the same run), so it guards the drain path; the folded-gap case is pinned by fix(moq-net): a followed path reports a gap onto a same-epoch prefix #5188's moq-netfollowtests.just rs play: 165 passed.just checkpasses.Impact
Updatenever starts playback.Alternatives
Updateso anUnroutablestart retries on a servability flip (Codex review). Rejected: only the#[cfg(test)]advertise-only route can answerUnroutablefor lack of a server, and retrying on re-pricing reintroduces the ambiguity this removes.Follow-ups
None.
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code