Repository navigation
fix(mux): bound TS import state by retiring ended routes and admitting SI on the program clock - #6757
Conversation
Quest-Claim: 51b74e8697d57927
…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>
…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>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 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".
kixelated
left a comment
There was a problem hiding this comment.
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.
|
No actionable comments were generated in the recent review.
|
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
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
💡 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".
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
left a comment
There was a problem hiding this comment.
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.
|
Landing summary for 8e23d82:
(Written by Claude Opus 5.5) |
|
Maintainer decisions (2026-10-25):
(Written by Claude Opus 5.5) |
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>
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.timelinesentry 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 whyImport::MAX_REVISIONSrefused 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'sarchive.timelinesentry.MAX_REVISIONSandcount_revisionare gone.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::pendingandEntry::activeforever. SI is now admitted on the program clock the way whole PES are (si::Clock):stats::Stream::unplaced, on a.sirow for its PID.Decisions
Decided by the maintainer on 2026-10-25:
<name>.timeline.zat record 0 for a fixed-name re-creation, reusing the name for different content.Choices made unattended
mux_rate, because a VBR source leaves that unset and would never age SI during a stall.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'simport_reports_count_what_each_rendition_wroteasserted every stats row delivers units. It now skips.sirows, which only count unplaced sections (bbb5s_cbr leads with an SDT before its first PCR).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.mpegts.tracksand 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
retire,si::Clockand the test-onlyTimelines::lenare crate-private.stats::Streamgains no field; an SI PID can now have a row.archive.timelinestogether with its rendition, instead of staying open for the life of the broadcast.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.sirow, 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_solicitswas SLOW at >30 s and then passed, as it was before this change. It doesn't exercise SI admission.cargo nextest -p moq-mux(1743 pass),cargo fmt --checkandclippy -D warningson moq-mux. The fulljust checkpassed on the previous head.just rs features, the platform lanes.Follow-ups
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code