Skip to content

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

Merged
kixelated merged 5 commits into
mainfrom
quest/m0/unpublish-clean
Nov 7, 2026
Merged

kixelated merged 5 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. served hands that group back, and ended reads 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

  • 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 at f9b8980, and 0 of 120 again at 0439d67 (which reads the first group).
  • nix develop --command quest gate just ci on nixos at f9b8980: passes (279 tests). Again at 0439d67, 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, one session:

  • Round 1 on 4d2de70: no findings.
  • Round 2 on head 0439d67445951820084eaf0ff7a8622e728da96a against origin/main at 79c60b9: no findings.

Online review of 4d2de70:

  • OpenAI, P2: served dropped the first group, so a cut in it went unnoticed. Fixed in 0439d67: ended reads that group first.

🤖 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 48 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: f76a6802-2c5e-4fee-8ecd-56331654b3fa
Commits

Reviewing files that changed from the base of the PR and between 79c60b9 and 0439d67.

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

kixelated and others added 2 commits November 7, 2026 06:47
…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>
@kixelated

Copy link
Copy Markdown
Contributor Author

Re the OpenAI P2: agreed and fixed in 0439d67. served now returns the first group with the subscriber, and ended reads that group to its end before receiving later groups, so a cut tail in the only group on the immediate-quit path fails the test again. Same stress repro on nixos (8 copies at once, lavapipe): 0 of 120. quest gate just ci: passes (281 tests). Local Codex round 2 on the new head: no findings.

(Written by Claude Opus 5.5)

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

@kixelated

Copy link
Copy Markdown
Contributor Author

Pick (/quest-complete, maintainer away under /afk, recommended option): ✅ Merge at 0439d67, once Check passes. The OpenAI P2 is fixed, and Codex round 2 found nothing.

(Written by Claude Opus 5.5)

@kixelated
kixelated merged commit 5e3e7fd into main Nov 7, 2026
3 checks passed
@kixelated
kixelated deleted the quest/m0/unpublish-clean branch November 7, 2026 15:15
kixelated added a commit that referenced this pull request Nov 7, 2026
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant