Skip to content

fix(hls): publish every rendition in the first catalog - #4861

Merged
kixelated merged 2 commits into
mainfrom
quest/m1/hls-first-catalog
Oct 6, 2026
Merged

kixelated merged 2 commits into
mainfrom
quest/m1/hls-first-catalog

Conversation

@kixelated

@kixelated kixelated commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

moq-hls import reserved the catalog only when a rendition's init segment loaded, inside TrackState::ensure_map. That reservation ends at the rendition's first frame, and renditions are ingested one at a time, so the first rendition could publish before the others existed. A consumer's first snapshot then listed only that rendition and grew on later updates. Reported by Dryvnt in #4824.

Approach

step takes a catalog Reserved after ensure_tracks and before the first ingest, and drops it when the pass ends. The first catalog therefore lists every rendition that loaded its init segment in that pass.

Every pass holds, not only the first. A quiet playlist publishes nothing, so the first catalog may be a later pass. Once the catalog has been published, a reservation no longer gates updates, so later passes are not withheld. A rendition with no segments yet, or whose init fetch fails under OnError::Warn, never reserves and cannot withhold the catalog past that pass.

Main no longer reserves the timeline across a pass (#4034). This change does not put that hold back.

Impact

  • Public API: none.
  • Wire: none. A multi-rendition HLS import's first catalog lists every rendition that loaded its init segment in that pass, instead of only the first.

Alternatives

Hold only on the first pass. That still publishes a partial catalog when the first pass is quiet and the renditions load together on the next one.

Follow-ups

A later pass can still publish before a rendition that has no media yet, because that rendition never reserves. That is the bound this quest set.

Import at the first frame has landed (#4885). Each importer still releases at its first frame, before the next rendition is reserved; the pass hold covers that gap and drops at the end of the pass.

(Written by Grok 4.7)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Quest quest/m1/hls-first-catalog is implemented and the quest file is deleted.

The regression test fails on main with a first catalog of video 1 / audio 0, and passes with the pass-wide hold. just check ran moq-hls (233) and moq-cli (143) tests under clippy; the only failure was remark on a pre-existing untracked .worktrees/ directory, which is not part of this diff.

Left as a draft. Holding on every pass, not only the first, is the choice worth a look: a quiet first pass publishes nothing, so the first catalog can be a later pass.

(Written by Grok 4.7)

kixelated and others added 2 commits October 6, 2026 07:53
Co-authored-by: Grok 4.7 <noreply@x.ai>
An HLS import reserved the catalog only when each rendition's init segment loaded, and that reservation ended before the next rendition existed, so the first snapshot listed only the first rendition. Hold a catalog reservation for the whole pass, beside the timeline reservation, and drop it when the pass ends.

Co-authored-by: Grok 4.7 <noreply@x.ai>
@kixelated
kixelated force-pushed the quest/m1/hls-first-catalog branch from 669ab69 to d7f2921 Compare October 6, 2026 14:58
@kixelated
kixelated marked this pull request as ready for review October 6, 2026 14:59
@kixelated

Copy link
Copy Markdown
Collaborator Author

Rebased onto main (32441439). The quest file is deleted, and both sides' quest/m1/README.md bullet removals are kept.

The HLS importer holds a catalog reservation across each pass, so the first catalog lists every rendition that loaded in that pass. Each importer still releases its own reservation at its first frame; the pass hold is what covers the gap. Main removed the pass-wide timeline reservation in #4034, and this does not restore it. No public API change and no wire change: the catalog that is pushed is the one consumers read.

Confirmed: hold across the pass, not only the first. A rendition with no media yet still cannot withhold the catalog past that pass.

(Written by Grok 4.7)

@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: c882f9c3-aaf0-4c52-8737-b12a54293946
📥 Commits

Reviewing files that changed from the base of the PR and between 3244143 and d7f2921.

📒 Files selected for processing (3)
  • quest/m1/README.md
  • quest/m1/hls-first-catalog.md
  • rs/moq-hls/src/import.rs
💤 Files with no reviewable changes (2)
  • quest/m1/hls-first-catalog.md
  • quest/m1/README.md

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


Walkthrough

The HLS importer now holds a catalog reservation across a pass over active tracks. A regression test gates the audio playlist response and checks that the first catalog snapshot includes both video and audio renditions. The Required quest entry and its proposal document were removed.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to d7f29

The catalog change has no identified merge-blocking issue. Normal checks remain appropriate before merging.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 1 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main change: publishing every eligible rendition in the first HLS catalog.
Description check ✅ Passed The description explains the catalog reservation change, its intended behavior, and its impact.
✨ Finishing Touches
✨ Simplify code
  • Commit to this branch
  • Create a new PR
  • 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

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

@kixelated

Copy link
Copy Markdown
Collaborator Author

Automated review: fix(hls): publish every rendition in the first catalog

Reviewed head: d7f29210

Holding a Reserved across each step pass fixes the partial first catalog in a small way. I checked moq-mux's flush_if_ready (rs/moq-mux/src/catalog/producer.rs:621-644): once published is set, later reservations are no-ops, so taking one on every pass only gates the first publish, as the description says. The early-return paths (ensure_tracks()?, OnError::Fail) drop the guard and can't leak it. CI is green and the PR is mergeable.

Findings (by severity)

  1. Medium: one slow or hung rendition now holds back the whole first catalog. rs/moq-hls/src/import.rs:732 holds the reservation for the entire pass, and the pass is sequential (:739-769). Each rendition fetches its playlist, init segment, and up to ANCHOR_SEGMENTS (3) segments one after another, and each request has a 30 s REQUEST_TIMEOUT (:30). Under run() (OnError::Warn), if the audio playlist or one variant's init or segment hangs until it times out, the catalog stays unpublished for that long (30 s or more per stalled request). Meanwhile video frames that already ingested are being written to tracks nobody can discover yet. Before this change, video published at its first frame, and a hung audio rendition delayed only audio. Even on a healthy origin, a wide ABR ladder now waits for every variant's anchor window to download serially before anything appears. Possible fix: bound the hold. For example, give the pass reservation a deadline (a few seconds, or about one target duration) and drop it when the deadline passes, or fetch every rendition's playlist and init concurrently before the serial segment ingest. If the unbounded wait is intended, it's worth saying in the doc comment that the bound is "the pass, including request timeouts".

  2. Low: the regression test hangs instead of failing if the gate server breaks. serve_gated unwraps inside a spawned task (:1664, plus the panic! for unknown paths at :1690). If that task panics, arrived.notified().await (:1623) never wakes and the test hangs until the CI job times out. Wrapping that await, and importing.await, in tokio::time::timeout would make it fail fast with a clear message.

  3. Nit: on the non-early path (:1640-1648), the final check reads the newest snapshot, which on its own can't tell a first catalog apart from a later update that grew. The early read is what actually proves the fix, and that's fine. A short note saying so would keep a later refactor from dropping the early read as redundant.

Verdict

MERGE. The fix is correct and well tested. Finding 1 is a real latency tradeoff on a degraded origin, so either bound the hold or confirm the unbounded first-pass wait is acceptable. Reviewed head d7f29210.

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

@kixelated

Copy link
Copy Markdown
Collaborator Author

On the automated review of d7f292101:

  1. The pass hold stays unbounded. A deadline, or publishing as soon as one rendition's first frame lands, is the partial first catalog this quest removes. A rendition that never loads still cannot withhold the catalog past the pass: the reservation drops when step returns, including after OnError::Warn. A hung fetch already stalled that rendition's own publish for the request timeout; the pass now waits with it so the first catalog is complete. Confirmed decision: hold across the pass.

  2. Leaving the test without a timeout. arrived only fails to fire if the gate server panics, and the existing one-shot import servers fail the same way. A timeout would not change the catalog behavior and would move the head.

  3. Leaving the non-early assertion as it is. The comment above the test already says the early read is the first catalog when the pass publishes before the second rendition. The later read only covers a pass that held until the end, which is the path this reservation takes.

(Written by Grok 4.7)

@kixelated
kixelated merged commit 296e4c0 into main Oct 6, 2026
7 checks passed
@kixelated
kixelated deleted the quest/m1/hls-first-catalog branch October 6, 2026 15:35
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