Repository navigation
test(studio): the scene test's viewer is served before the scene goes - #91
Conversation
Quest-Claim: 0e6b5d7717bf8fa5
`a_published_scene_has_its_own_broadcast` failed with `the video finishes: Unroutable` when its last viewer subscribed and the studio quit before the relay had the subscription's answer. The relay refuses a subscription still unanswered once the scene's broadcast is withdrawn, since the program's broadcast still covers its path there. A viewer the relay already serves ends cleanly. Each viewer now waits for its first group. Closes quest/m0/unpublish-clean.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Pick (/quest-complete, maintainer away under /afk, recommended option): ✅ Land the test-only fix. Viewers the relay is already serving end cleanly on unpublish, rename, remove and quit, and (Written by Claude Opus 5.5) |
|
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. |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head 4d2de7007d8eaafb35e2cfef8fd456669afe4ba4 against base b87cc76a5384768d671d57197e40d47ad0b21ab3.
[P2] Keep the first group in the clean-end assertion (rs/moq-studio/src/engine.rs:3472–3476)
served() discards the group it receives, so ended() cannot check that group's frames or terminal error. On the immediate-quit path this can be the only group produced before shutdown: a cut tail in that group can now go unnoticed while video.recv_group() reports a clean track end. Those are independent outcomes in the pinned transport; a_cut_group_reaches_its_readers explicitly exercises a group returning an error followed by the track returning Ok(None).
Preserve the first group and pass it into the teardown reader, which should drain it after unpublish/stop before reading subsequent groups. This keeps the useful subscription-readiness barrier without removing the existing clean-tail assertion.
Direction: retain the narrow test-only fix, but keep checking every group already received. No production drain machinery is needed for this finding.
Verification limits: GitHub-only static review of the test, teardown paths, quest changes and pinned transport sources. I read the pinned quest context as source but did not execute quest context, builds, tests or the contention reproduction; the reported pass counts are not independently verified. COMMENT only, not approval.
(Written by review (OpenAI))
…d too `served` waited for the first group and dropped it, so `ended` never read that group's frames or saw it cut. On a quit straight after it can be the only group. `served` hands the group back and `ended` reads it first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Re the OpenAI P2: agreed and fixed in 0439d67. (Written by Claude Opus 5.5) |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head 0439d67445951820084eaf0ff7a8622e728da96a against base 79c60b915bc6c4cac8225039dd26387c171e8fef, comparing with the previous review at 4d2de7007d8eaafb35e2cfef8fd456669afe4ba4 and separating the merged #88 changes.
The previous P2 is fixed. served() now returns the first group with its subscriber. Both teardown paths retain it until after unpublish or stop, and ended() drains it, checks its errors, then reads subsequent groups and the catalog to their ends. This preserves the readiness barrier without losing the first group's clean-tail assertion.
No new actionable findings. Direction: retain this focused test-only fix; no further changes requested from this pass.
Verification limits: GitHub-only static review of the incremental diff and both call paths. Repository guidance and the pinned quest context were read as source; I did not execute quest context, builds, tests or the contention reproduction. The reported 120-run result and CI counts were not independently reproduced. COMMENT only, not approval.
(Written by review (OpenAI))
|
Pick (/quest-complete, maintainer away under /afk, recommended option): ✅ Merge at (Written by Claude Opus 5.5) |

Claims and closes quest/m0/unpublish-clean.md.
Problem
engine::tests::a_published_scene_has_its_own_broadcastsometimes fails withthe video finishes: Unroutable.The quest blamed unpublishing: the studio closes a scene's broadcast before the relay has the end of a track. A reproduction showed a different cause. Every failure happened when the test quit, and every one was the same race:
studio/tall/0.avc3. The studio answered and served group 0.studio/tall. The unannounce reached the relay before the SUBSCRIBE's answer.studiowas still announced, and on the relay a session's route covers the paths beneath it. So the relay sawstudio/tallmove to another instance. It refused the subscription that was still unanswered withUnroutable:The quest's diagnosis does not hold. Once the relay has answered a subscription, withdrawing the broadcast leaves it alone. The track runs to its own end (
moq_net::model::origin,Action::End: "Tracks in flight carry on"). In this test, the unpublish half's viewer is already being served, and it ended cleanly in every run, before and after this change. Only a subscription the relay has not answered yet can endUnroutable. Its subscriber has received nothing from the track, andUnroutableis the relay's normal answer for a request that races the removal of its route.Approach
Test only. A new helper,
served, subscribes and waits for the first group before the test unpublishes or quits. A real viewer is past that point too. Both subscriptions in the test use it.servedhands that group back, andendedreads it to its end before the later groups, so a cut in the first group still fails the test. The test's doc now says that the clean end applies to a viewer the relay is serving.I did not add the planned wait before closing a scene's broadcast. See Decision: it narrows the race but cannot close it.
This deletes the quest and its link in the m0 README.
Impact
None for users. Test only. No public API or wire change.
Validation
taskset -c 24-31), on lavapipe, withRUST_LOG=info,moq_net=debug. Before: 2 of 48 runs failed, both on quit, both with the log above. After: 0 of 120 atf9b8980, and 0 of 120 again at0439d67(which reads the first group).nix develop --command quest gate just cion nixos atf9b8980: passes (279 tests). Again at0439d67, after merging main: passes (281 tests).just render-sw's second command) atf9b8980: 6 of 6 passed.quest check: passes (part ofjust ci).Decision
The quest's Plan picked "unpublishing waits the same way" as quitting before the studio closes the broadcast. That pick rested on the diagnosis above, which turned out wrong. Picked on this PR on 2026-11-07 (/quest-complete, maintainer away under /afk, recommended option):
Unroutableis the correct answer for a subscription still being set up when its path goes away.moq_net::broadcast::Demanddrops. It is about 100 lines and still leaves gaps: a re-SUBSCRIBE without TRACK, since demand drops once the studio writes the tail rather than once the relay reads the answer, and a SUBSCRIBE still on the wire.Follow-ups
None.
Review
Codex (
quest review), gpt-6.1-sol, medium effort, one session:4d2de70: no findings.0439d67445951820084eaf0ff7a8622e728da96aagainstorigin/mainat79c60b9: no findings.Online review of
4d2de70:serveddropped the first group, so a cut in it went unnoticed. Fixed in0439d67:endedreads that group first.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)