Skip to content

test(studio): the scene test's viewer is served before the scene goes - #91

Open
kixelated wants to merge 3 commits into
mainfrom
quest/m0/unpublish-clean
Open

kixelated wants to merge 3 commits into
mainfrom
quest/m0/unpublish-clean

Conversation

@kixelated

@kixelated kixelated commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

Claims and closes quest/m0/unpublish-clean.md.

Problem

engine::tests::a_published_scene_has_its_own_broadcast sometimes fails with the 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:

  1. The test subscribed to the scene's video on the relay, then quit at once.
  2. The relay sent the studio TRACK and SUBSCRIBE for studio/tall/0.avc3. The studio answered and served group 0.
  3. The studio stopped. Its encoders finished their tracks and it unannounced studio/tall. The unannounce reached the relay before the SUBSCRIBE's answer.
  4. The program's broadcast studio was still announced, and on the relay a session's route covers the paths beneath it. So the relay saw studio/tall move to another instance. It refused the subscription that was still unanswered with Unroutable:
DEBUG moq_net::lite::publisher: serving group subscribe=5 track=0.avc3 sequence=0
DEBUG moq_net::lite::publisher: unannounce route=studio/tall
DEBUG moq_net::lite::subscriber: unannounced broadcast=studio/tall
DEBUG moq_net::lite::subscriber: unanswered subscribe refused: instance replaced broadcast=studio/tall track=0.avc3
DEBUG moq_net::lite::publisher: track info complete broadcast=studio/tall track=0.avc3
panicked at rs/moq-studio/src/engine.rs:3462:66: the video finishes: Unroutable

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 end Unroutable. Its subscriber has received nothing from the track, and Unroutable is 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. 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

  • Reproduced on nixos with moq-dev/studio#89's method: 8 copies of the test at once, pinned to 8 cores (taskset -c 24-31), on lavapipe, with RUST_LOG=info,moq_net=debug. Before: 2 of 48 runs failed, both on quit, both with the log above. After: 0 of 120.
  • nix develop --command quest gate just ci on nixos at f9b8980: passes (279 tests). Again at 4d2de70, after merging main: passes (281 tests).
  • The nightly engine set (just render-sw's second command) at f9b8980: 6 of 6 passed.
  • quest check: passes (part of just ci).
  • Not run: macOS. The test is Linux lavapipe only.

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):

  • ✅ Land the test-only fix. A viewer the relay serves already gets a clean finish when a scene is unpublished, renamed, or removed, or when the studio quits. Unroutable is the correct answer for a subscription still being set up when its path goes away.
  • Rejected: the studio waits before closing a scene's broadcast until moq_net::broadcast::Demand drops. 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.
  • Rejected: a moq-net change, either a publisher session holding a broadcast's unannounce until its in-flight subscriptions are answered, or the relay leaving an unanswered SUBSCRIBE to its answer when its path falls back to a covering route from the same session. It overlaps the routing rework in moq-dev/moq#8023.

Follow-ups

None.

Review

Codex (quest review), gpt-6.1-sol, medium effort. Round 1 on head 4d2de7007d8eaafb35e2cfef8fd456669afe4ba4 against origin/main at b87cc76: no findings.

🤖 Generated with Claude Code

(Written by Claude Opus 5.5)

@pullfrog

pullfrog Bot commented Nov 7, 2026

Copy link
Copy Markdown

Pullfrog  | Review this ➔

`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>
@kixelated kixelated changed the title Unpublishing a scene ends its broadcast cleanly test(studio): the scene test's viewer is served before the scene goes Nov 7, 2026
@kixelated

Copy link
Copy Markdown
Contributor Author

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 Unroutable is a correct answer for a subscription still being set up when its path goes away. Rejected: a studio-side wait (about 100 lines and still leaves gaps) and a moq-net change (overlaps moq-dev/moq#8023's routing rework). The catalog that ends without finish() on quit gets planned as its own quest.

(Written by Claude Opus 5.5)

@kixelated
kixelated marked this pull request as ready for review November 7, 2026 14:32
@coderabbitai

coderabbitai Bot commented Nov 7, 2026

Copy link
Copy Markdown

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 5 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: e801c434-d20e-4a76-95e6-df82a5982433
Commits

Reviewing files that changed from the base of the PR and between b87cc76 and 4d2de70.

Files selected for processing: 3

  • quest/m0/README.md
  • quest/m0/unpublish-clean.md
  • rs/moq-studio/src/engine.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

@kixelated kixelated left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant