Skip to content

fix(mux): bound TS import state by retiring ended routes and admitting SI on the program clock - #6757

Merged
kixelated merged 7 commits into
mainfrom
quest/m1/ts-reliability/state-bound
Oct 25, 2026
Merged

kixelated merged 7 commits into
mainfrom
quest/m1/ts-reliability/state-bound

Conversation

@kixelated

@kixelated kixelated commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator

Finishes State bound.

Summary

A TS import's retained state is now bounded by what its source repeats, including SI that arrives without program clock progress.

Ended routes. A TS route mints its own track name, so once the route ends, nothing enrolls that name again. The timeline registry still kept every ended route's timeline: an open timeline track, its segmenter, and (once declared) an archive.timelines entry in every catalog republish. Every enrollment also rebuilt the archive section over the whole registry, so a source revising its PMT grew the broadcast without bound and cost quadratic time. That's why Import::MAX_REVISIONS refused the import after 65,536 revisions.

  • timeline::Timelines::retire(name) finishes a dropped recorder's timeline track, so readers already on it read it to its end, and forgets it. It errors while the recorder is still alive.
  • catalog::Producer::retire(track) calls that and drops the track's archive.timelines entry.
  • The TS import retires the track name of every route it ends (PMT revision, PCR-loss refusal, section remap). No other per-revision owner is left, so MAX_REVISIONS and count_revision are gone.
  • Already bounded: BroadcastState::sequences, which the quest named, was removed by moq-dev/moq#6428.

SI without clock progress. Standalone SI used to be accepted before any PCR and aged only on accepted PCR steps, so a source sending fresh EIT identities with a frozen clock grew Entry::pending and Entry::active forever. SI is now admitted on the program clock the way whole PES are (si::Clock):

  • SI before the first PCR, or past it with no PCR interval to extrapolate from, is dropped and counted in stats::Stream::unplaced, on a .si row for its PID.
  • A valid PCR interval (forward, at most 1 s) advances the SI clock by its own ticks and joins the measured multiplex rate. A stalled, rewound, or corruptly jumping PCR, or an interval across a clock break, advances it by the input packets it spans at the measured rate. A valid step that resumes after PCR silence keeps the age the silence already charged. The open interval past the last PCR does too, once it outruns any valid interval. So SI keeps aging while the PCR stalls or goes silent.

Decisions

Decided by the maintainer on 2026-10-25:

  1. Timeline retirement contract (the quest's "retired route names and archives" item).
    • (a) The owner retires explicitly, as implemented here. A dropped recorder keeps re-enrollment continuity. (recommended) ✅
    • (b) A timeline retires implicitly when its rendition or catalog entry leaves. This would restart <name>.timeline.z at record 0 for a fixed-name re-creation, reusing the name for different content.
    • (c) Mint timeline track names per enrollment. HLS media-sequence continuity across a re-enrollment breaks.
  2. SI without clock progress.
    • (a) Admit SI on the program clock as whole PES are: drop it and count it unplaced before the first PCR, and while the PCR stalls, age it on the input position extrapolated at the measured mux rate. (recommended) ✅
    • (b) Narrow the Goal to sources whose clock progresses.

Choices made unattended

  • Retirement is explicit and comes from the owner. Only an owner that mints one name per track (the TS import) retires names, and ownership is never inferred from a name or prefix. End of input is unchanged: a finished import's timelines stay advertised.
  • The SI clock's rate pools every valid PCR interval since the import started. It does not read the catalog's mux_rate, because a VBR source leaves that unset and would never age SI during a stall.
  • SI after the first PCR but before any valid interval is also dropped as unplaced, as a whole PES with no interval to extrapolate from is. Without a rate, a source that stalls right after its first PCR would hold that SI forever. On a conformant stream this window is one PCR interval (100 ms or less).
  • The open interval is charged only past MAX_INTERVAL (1 s), the longest valid PCR interval. Until then the next PCR may still close it, and extrapolating a VBR burst above the average rate would retire SI early.
  • moq-cli's import_reports_count_what_each_rendition_wrote asserted every stats row delivers units. It now skips .si rows, which only count unplaced sections (bbb5s_cbr leads with an SDT before its first PCR).
  • The two export round-trip tests (service_layer_survives_roundtrip, a_selected_programs_si_survives_export) now read the exported SI off the wire instead of re-importing it. The export leads with SI before its first PCRs, so a re-import drops each table's first emission until it repeats (see Follow-ups). The import-side SI test rigs start with a PAT, a PMT, and one PCR interval.
  • The churn test runs 999 revisions and asserts flat registry, archive, mpegts.tracks and route state. It does not run past 65,536: with the constant deleted, flatness is the property, and 999 revisions finish in about 50 ms.

Impact

  • Public API: none. retire, si::Clock and the test-only Timelines::len are crate-private. stats::Stream gains no field; an SI PID can now have a row.
  • Wire: none.
  • Behavior:
    • A TS import no longer refuses a source after 65,536 PMT revisions. An ended route's timeline track now finishes and leaves archive.timelines together with its rendition, instead of staying open for the life of the broadcast.
    • SI that arrives before the program clock can place it is dropped and counted unplaced. Sources such as ffmpeg lead with an SDT before their first PCR, so each SI table is first carried at its next repetition: up to 2 s for an SDT, 10 s for a NIT, 30 s for a TDT/TOT. While the PCR stalls, SI now retires on schedule instead of being held.

Tests

On the mocked clock (#[moq_clock::test]; the SI clock is the program clock, never host time):

  • program_clock_places_and_retires_si_without_media_pts: SI before the first PCR and before the first interval is dropped and counted on the .si row, then placed and retired once an interval exists.
  • fresh_si_ages_out_while_the_pcr_stalls: 2,000 fresh EIT identities, one per 40 ms of input, under a PCR repeating one value and under a silent PCR. Pending state settles at the table's 2 s window (51) in both cases. The silent case peaks once at 75, because the open interval's first second is not charged.
  • the_clock_counts_valid_intervals_and_extrapolates_the_rest: wrap, stall, rewind, corrupt jump, flagged break, the open interval, and a small step resuming after 10 s of PCR silence. That last case is the Codex finding moq-dev/moq#6757 (comment): it used to freeze SI aging for as long as the silence lasted.
  • the_clock_follows_a_rate_increase: a 26x bitrate rise. Each valid step still counts as its own 40 ms, per the Codex finding moq-dev/moq#6757 (comment).

Checks

  • just check: pass. moq-cli::integration report::export_echo_reports_where_the_import_solicits was SLOW at >30 s and then passed, as it was before this change. It doesn't exercise SI admission.
  • I reproduced the PMT-revision bug first: with retirement disabled, the churn test fails with 2001 timelines against 5.
  • After the Codex fixes: cargo nextest -p moq-mux (1743 pass), cargo fmt --check and clippy -D warnings on moq-mux. The full just check passed on the previous head.
  • Not run: just rs features, the platform lanes.

Follow-ups

  • The export writes its first SI ahead of the PMT and the PCRs that follow it, so an export to TS import hop loses each table's first emission until it repeats. The export could hold its first SI until its PMT and one PCR interval are out.
  • The SI clock is bounded only relative to the source's own clock. A source whose valid PCR steps advance a tick at a time across many packets, or that resumes from long PCR silences with small steps, claims an enormous mux rate and still ages SI slowly. Bounding that needs a floor on the program clock per input byte, which would be a new contract.
  • fMP4/MKV re-init paths that mint a fresh name per init could retire their old timelines the same way, if a source can re-init without end.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

kixelated and others added 2 commits October 25, 2026 01:53
…sions

Every TS route mints its own track name, so once the route ends nothing
enrolls that name again. The timeline registry still kept each one (an open
timeline track plus its archive entry), so a source revising its PMT grew the
broadcast without bound, and `Import::MAX_REVISIONS` refused it after 65,536
revisions. `catalog::Producer::retire` now finishes the ended route's timeline
track for the readers on it and drops it from the registry and the root
`archive` entry, so a churning source holds only its live routes, and the
guard goes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated kixelated changed the title TS import holds only what its source keeps repeating fix(mux): retire an ended TS route's timeline instead of capping revisions Oct 25, 2026
kixelated and others added 3 commits October 25, 2026 08:35
…ld it

SI before the program clock can place it is dropped and counted unplaced,
and while the PCR stalls it ages on the input position extrapolated at the
measured multiplex rate. Finishes the state-bound quest.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated kixelated changed the title fix(mux): retire an ended TS route's timeline instead of capping revisions fix(mux): bound TS import state by retiring ended routes and admitting SI on the program clock Oct 25, 2026
@kixelated
kixelated marked this pull request as ready for review October 25, 2026 16:38
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-25T16:56:36.478874Z 8e23d82 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 96c733e461

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread rs/moq-mux/src/container/ts/si.rs Outdated

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator 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 commit: 96c733e

Explicit retirement of uniquely named TS routes is a focused replacement for the revision cap. SI admission on the program clock matches the chosen contract.

Confirmed the existing P1 PCR-resumption finding #6757 (comment) at rs/moq-mux/src/container/ts/si.rs:473–476: preserve extrapolated SI age when accepting resumed PCR, and test silence → small forward PCR → continued SI. Otherwise expiry freezes for a source-controlled interval. Referencing the existing finding rather than duplicating inline; no additional actionable bugs found.

Verification: static changed and surrounding code inspection. Tests were not run.

@coderabbitai

coderabbitai Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent 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: 8d0b88f4-7b19-4d3c-bd31-0942c40834a9
Commits

Reviewing files that changed from the base of the PR and between 96c733e and 971b70c.

Files selected for processing: 1
  • rs/moq-mux/src/container/ts/si.rs

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

Walkthrough

The importer now measures SI time from PCR samples and packet counts. It drops and counts SI sections that cannot be placed. Timeline and catalog retirement now support route endings, refusals, and route changes. The importer no longer enforces a PMT revision limit. Tests and documentation cover SI placement, statistics, export, and repeated route revisions.

Priority: Normal

Merge Risk: Minimal · up to 971b7

The SI clock change looks safe to merge. One narrow edge case remains: if a stream's packet rate jumps by more than tenfold and stays there, SI could age slightly too fast and be retracted early. This does not need to block the merge.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 70.15% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 67 functions across 8 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly summarizes the two main changes: retiring ended TS routes and admitting SI on the program clock to bound import state.
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.
Description check Passed The description is specific and on-topic. It names the State bound quest and explains the changes: retiring ended TS route timelines, removing MAX_REVISIONS, and admitting SI on the program clock. It …
Full details: Description check

Explanation

The description is specific and on-topic. It names the State bound quest and explains the changes: retiring ended TS route timelines, removing MAX_REVISIONS, and admitting SI on the program clock. It also covers the tests and the validation done. It matches the changeset summary, which covers timeline retirement in timeline.rs and catalog/producer.rs, SI clock changes in si.rs, the import.rs changes, and the moq-cli report test skip for .si rows.

Finishing Touches
✨ Simplify code
  • Commit to this branch
  • Create a new PR
  • 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

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

An interval spanning more input than MAX_INTERVAL at the measured rate is
charged its extrapolation, not its PCR step, so a small step after a long
silence no longer drops the age the open interval already charged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator 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 commit: 971b70c

The PCR-resumption finding #6757 (comment) is fixed: long silent intervals retain extrapolated age without contaminating measured rate, and subsequent PCR steps keep advancing SI expiry. Regression assertions cover resumption and continued aging.

No new actionable bugs found. This is a focused correction consistent with bounded-state design.

Verification: static comparison against 96c733e and surrounding clock/admission logic. Tests were not run.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 971b70ca47

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread rs/moq-mux/src/container/ts/si.rs Outdated
Every valid PCR step joins the measured rate again; closing an interval
charges the larger of its step and what the open interval already charged,
so PCR silence keeps its age without locking the rate after a bitrate rise.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator 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 commit: 8e23d82

Compared with 971b70c. The VBR rate-recovery finding #6757 (comment) is fixed: valid PCR intervals update the measured rate again, while retaining previously charged open-interval age keeps the earlier PCR-resumption freeze fixed.

No new actionable bugs found. This focused correction and its rate-increase/resumption regression coverage are appropriate. SI aging still depends on the source clock; the documented slow-clock limitation is not an absolute input-byte memory bound.

Verification: static inspection of the changed clock arithmetic, surrounding admission/expiry logic, and regression assertions. No tests or builds run.

@kixelated
kixelated added this pull request to the merge queue Oct 25, 2026
@kixelated

Copy link
Copy Markdown
Collaborator Author

Landing summary for 8e23d82:

  • The owner retires an ended TS route's timeline explicitly (maintainer decision 1a), replacing MAX_REVISIONS.
  • SI is admitted on the program clock (maintainer decision 2a). It is dropped and counted unplaced before the clock can place it. While the PCR stalls or goes silent, it ages on input position extrapolated at the measured mux rate.
  • Two Codex findings fixed and resolved: aging froze when the PCR resumed after silence (thread), and the rate locked after a bitrate rise (thread).
  • Checks: just check passed on 96c733e. After that, cargo nextest -p moq-mux, fmt, and clippy passed on the final head. Required CI is green, and Codex gave the final head a thumbs-up.

(Written by Claude Opus 5.5)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Maintainer decisions (2026-10-25):

  1. Timeline retirement: land moq-dev/moq#6783 (catalog-hls) first. It deletes rs/moq-mux/src/timeline.rs, so drop the timeline retirement work here and link catalog-hls from the quest.
  2. SI without clock progress: option (a). Before the first PCR, drop SI and count it as unplaced. During a PCR stall, age it on the input position at the measured mux rate.

(Written by Claude Opus 5.5)

Merged via the queue into main with commit c38f57e Oct 25, 2026
10 checks passed
@kixelated
kixelated deleted the quest/m1/ts-reliability/state-bound branch October 25, 2026 18:42
kixelated added a commit that referenced this pull request Oct 25, 2026
Ports main's changes onto per-rendition HLS records:
- #6757: `retire` moves to `hls::Tracks::retire`, and the TS PMT churn test counts record tracks.
- #6777 and #6776: the recorder and session tests publish with `with_hls`, read stops as gaps, and take segment tags from the rendition.
- The segment cursor yields gaps, as main now does.

A reconfigured rendition's segment tag now folds in its normalized config, and its replacement lists only the records after its predecessor's, so one URL never names two muxes. A dropping pacer no longer makes followers stop early.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant