[kixelated]

Problem

moq-scene source::tests::a_replacement_track_keeps_its_delay (from #38) hit nextest's 120 s TIMEOUT on Linux CI. It happened in run 41979555980 (PR #43) and run 41991220605 (PR #44), and both passed on a re-run.

Root cause: the test replaced the video rendition by dropping the old encode::Producer unfinished, which aborts its track. Two events then race:

• the old decoder sees the abort (moq: not found). play() treats that as a failed instance, so the source goes Failed and waits for the broadcast to restart, which never happens in the test;
• the catalog update that names the replacement. It is withheld until the replacement's first frame publishes its config, and when it wins, the old decoder is dropped before its error surfaces.

When the abort won, the replacement was never opened, and the test's until spun forever. Instrumented logs from a hung run:

CATALOG video ["0.avc3"] ...
OPEN video 0.avc3 ...
STEP 4 (replacement picture published)
PLAY ended Some("moq: not found")

A passing run instead shows CATALOG video ["1.avc3"] and OPEN video 1.avc3.

I checked both suspects named in the brief:

• Threads tokio can't see. They do move the paused clock a lot: the mid-test encoder probe advanced it by about 2,000 to 9,000 s in passing and hanging runs alike. That jump does not cause the hang.
• openh264 skipping frames. It didn't skip any: in every hung run the replacement frame was encoded and published. Its track was simply never opened.

Approach

The test now retires the old rendition the way a publisher does: finish() it, then drop it. A finished track ends the old decoder cleanly (Ok(None)), and play() waits for the next catalog update, as "a rendition that ends waits for a catalog update to replace it" says. No timeout was changed, and the product code is untouched.

Validation

On the NixOS host (32 cores), I ran the test binary with --exact 2,000 times, 32 at a time, each capped at 20 s:

	passed	hung (exit 124)
before (main 2a90a73)	1891	109
after (this PR)	2000	0

just ci passes (207 tests).

Impact

• Test only. There are no user-visible, public API, or wire changes.

Alternatives

• Make play() survive a rendition track that errors and wait for a catalog update instead. I didn't: an aborted track is a real failure, and failing loudly matches the repo rules. A well-behaved publisher names the replacement first and finishes the old track, which moq play also relies on (moq/rs/moq-cli/src/play/media.rs:188-193). Recommendation: keep it as is. If the maintainer wants sources to ride out an abrupt rendition loss, that is its own quest.

Follow-ups

• None.

Review

Codex (quest review), gpt-6.1-sol, medium effort, round 1 on head ebc3484: no findings.

🤖 Generated with Claude Code

[coderabbitai]

Walkthrough

The replacement video rendition test now calls finish() on the old video producer before dropping it. A comment states that dropping an unfinished producer aborts the track and may fail the source before the replacement catalog update arrives.

Merge Risk: Minimal · up to 8ecfd

This test-only change introduces no established product failure. It is mergeable after normal checks; the possible timing-coverage gap remains unverified.

Pre-merge checks: 5

Check name	Status	Explanation
Title check		The title clearly describes the main test fix: retiring the replaced rendition cleanly in the flaky replacement-delay test.
Description check		The description directly explains the timeout problem, root cause, implementation, validation, and test-only impact.
Docstring Coverage		No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
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.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files.

Finishing Touches

Simplify code

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Some tools did not complete. Review the errors below.

Clippy (1.99.1)

Clippy execution failed

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

• X
• Mastodon
• Reddit
• LinkedIn

[pullfrog]

✅ No new issues found.

Reviewed changes

Reviewed the test-only change to rs/moq-scene/src/source.rs at ebc3484, including Source::play/play_video and moq_video::encode::Producer::finish at the pinned moq commit.

• Clean retirement in a_replacement_track_keeps_its_delay — the old rendition is now finish()ed before it is dropped, so its track ends with Ok(None) rather than aborting, and play waits for the catalog update that names the replacement. This matches the real publisher path: publish.rs's Encoding::end/Drop finishes the producer before it goes.

Process note: quest context is unavailable in this review environment (no quest or nix on PATH), so I read AGENTS.md, CONTRIBUTING.md, and REVIEW.md directly, as REVIEW.md asks.

  ^{｜ View workflow run ｜ Using big-pickle (free) ｜ 𝕏}

[coderabbitai]

Additional comments: 1

rs/moq-scene/src/source.rs-2325-2325: 1

2325-2325: Minor · Potential issue · Low value

Unverified finding
We couldn't verify this from the code. It is shown for review, not as a verified issue.

Test gap: finishing the old producer may deliver a decoder End event that resets the video clock before replacement, allowing the delay test to pass without resetting the clock for the new track.

The test can pass despite a regression that shows replacement-track pictures too early when the catalog update arrives first.

This can occur when: The old decoder processes the finish event before the replacement catalog update, and the reset at the start of play_video is removed.

Review info

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: 90705f22-03f5-4d8e-b09e-613e2ab4f47b

Commits

Reviewing files that changed from the base of the PR and between e012e2e and 8ecfd98.

Files selected for processing: 1

• rs/moq-scene/src/source.rs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.