fix(moq-mux): refuse a moov or Tracks after the importer finished

[kixelated]

Problem

#4885 holds the catalog reservation until the first frame. Its review follow-up (3b8ad8d) made fMP4 take a fresh catalog.reserve() when a moov arrived after finish(), which declares tracks that can never finish. MKV had the same fallback (catalog.video/audio once the reservation was gone). Decision (2026-10-06): an init segment after finish() is an error.

Approach

• fMP4: init requires the initial reservation; before a moov only finish() can release it, so its absence means finished. Refused with fmp4::Error::MoovAfterFinish. The fresh-reserve() fallback is gone.
• MKV: same for the Tracks element, refused with mkv::Error::TracksAfterFinish before any track is added (per-track errors are only logged). The reservation is passed down, so the immediate-publish branch is gone.
• MPEG-TS has no such fallback: every stream takes its own fresh reservation by design, since PMTs may add streams mid-stream. Left alone (see Follow-ups).
• fix(moq-mux): importers publish the catalog at their first frame #4885 review items: upgrade.md says the catalog waits for media of a selected track; a with_select test pins that a deselected fragment arriving first doesn't publish; the MKV first-frame test asserts the Opus rendition; the moq-hls e2e test asserts its two discarded timeout results.

Tests a_moov_after_finish_is_refused (replaces a_moov_after_finish_publishes) and tracks_after_finish_are_refused both fail against main's import code.

Impact

• moq_mux::container::fmp4::Error::MoovAfterFinish: new variant (enum is #[non_exhaustive]).
• moq_mux::container::mkv::Error::TracksAfterFinish: new variant (enum is #[non_exhaustive]).
• Behavior: decoding a moov (fMP4) or Tracks (MKV) after finish() errors instead of declaring tracks. Noted in doc/setup/upgrade.md.
• No wire change.

Alternatives

• finish(self) consuming the importer would make this unrepresentable, but it is a wider API change (callers that abort or inspect after finishing).
• Refusing every decode after finish() would also cover TS, but goes beyond the decision.

Follow-ups

• Dropping or aborting an importer before its first frame (the reservation release on that path) is out of scope here.
• MPEG-TS still accepts a PMT that adds streams after finish(), declaring tracks that never finish. Not the reservation fallback, but the same outcome; worth deciding alongside the abort/drop path.

🤖 Generated with Claude Code

[coderabbitai]

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

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: d1619fa7-259f-4d55-878e-e33160aae147

📥 Commits

Reviewing files that changed from the base of the PR and between 06400a9 and c33c900.

📒 Files selected for processing (8)

• doc/setup/upgrade.md
• rs/moq-hls/src/export/mod.rs
• rs/moq-mux/src/container/fmp4/import.rs
• rs/moq-mux/src/container/fmp4/import_test.rs
• rs/moq-mux/src/container/fmp4/mod.rs
• rs/moq-mux/src/container/mkv/import.rs
• rs/moq-mux/src/container/mkv/import_test.rs
• rs/moq-mux/src/container/mkv/mod.rs

• Autopilot · 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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[kixelated]

Adversarial Codex review (branch diff against origin/main, head c33c900)

Verdict: approve. No material findings: the guards reject late track declarations before any catalog mutation.

(Written by Claude Opus 5.5)

[kixelated]

Automated review of c33c900 (fix(moq-mux): refuse a moov or Tracks after the importer finished)

This drops the "take a fresh catalog.reserve()" fallback in fMP4 init and MKV Tracks, so an init segment decoded after finish() now errors with fmp4::Error::MoovAfterFinish or mkv::Error::TracksAfterFinish instead of declaring tracks that could never finish. It's small and well targeted.

Blocking

None found.

I checked the invariant both new comments rely on ("only finish() releases the reservation before a moov/Tracks"):

• fMP4 (fmp4/import.rs): the reservation is otherwise cleared at the end of init when no track was declared, which sets self.moov (so a second moov hits DuplicateMoov first), or in extract at the first fragment. That needs a track in self.tracks, and extract already fails with NoMoov before reaching it. Skipped (deselected) track IDs continue before the release, which the new a_deselected_fragment_does_not_publish test pins.
• MKV (mkv/import.rs:263): the empty-tracks release sets tracks_seen, so a later Tracks never reaches the new check. The refusal also comes before handle_tracks, so no rendition is minted (the new test asserts that). self.handled is set before handle_tag, so a later decode replay won't re-emit the refused element.
• No in-tree caller decodes after finish(). moq-hls's importer replaces the whole Fmp4 per EXT-X-MAP instead of reusing a finished one, and import/container.rs just forwards. So the behavior change only affects external callers, and doc/setup/upgrade.md already flags it under the breaking-release section. Both new variants land on #[non_exhaustive] enums.

Non-blocking

1. MPEG-TS still declares never-finishing tracks when a PMT adds a stream after finish(). The PR already lists this as a follow-up. Just noting that the upgrade note only names fMP4 and MKV, so it's accurate as written.
2. moq-hls/src/export/mod.rs test: the new next_event(...).is_none() check is meaningful because renditions::Consumer replays the current set on first next(). So a moov that published early would show up as an Added within the 50 ms (paused clock) window. No change needed. Turning the discarded ready() timeout into an expect is a good tightening.

CI: Check, Test, Android, macOS and Windows were still queued at review time.

Verdict: MERGE (once CI is green)

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: c33c9000bb9d6c293da92a2b96e4107af6c35fe3

No actionable correctness findings. Direction is sound: the guards in rs/moq-mux/src/container/fmp4/import.rs:279-283 and rs/moq-mux/src/container/mkv/import.rs:263-267 reject late first initialization before declaring tracks. I traced reservation release at first selected media, empty selection, and finish; the new regressions cover the intended boundary and tighten the catalog/HLS assertions. This remains a targeted fix, with TS late-PMT behavior explicitly outside its scope.

Verification: static review of the full diff, relevant source context, and current discussion. I did not run tests or independently verify CI; this is not a merge approval.

(Written by OpenAI)