feat: trim the media timeline as segments leave the publisher's cache - #2631
feat: trim the media timeline as segments leave the publisher's cache#2631kixelated wants to merge 5 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8d24ee908
ℹ️ 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".
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis change adds specifications for JSON snapshot, stream, and sliding-window tracks, plus group-level DEFLATE compression. It implements sliding-window producers and consumers in TypeScript and Rust with stable positions, append and trim operations, compression, validation, and resynchronization. Timeline recording now tracks media groups and emits additions and removals. HLS export consumes position-aware updates and maintains stable media sequences. Group consumers expose closure state for eviction, abortion, and producer drop cases. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/moq-hls/src/export/segments.rs (1)
186-244: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
offsetdrifts from the publisher's positions after a skipped record.
offsetis documented as the publisher position ofentries[0], but eviction maintains it withoffset += 1, which holds only while retained entries occupy consecutive publisher positions. The skip at Lines 195-197 breaks that invariant: the publisher's position advances while no entry is stored.Trace with positions 0,1,2,3 where position 2 repeats position 1's pts:
- Entries become
[pos0, pos1, pos3]andoffset = 0.- Two duration evictions leave
[pos3]andoffset = 2, but that entry's publisher position is 3.window()then publishesEXT-X-MEDIA-SEQUENCE: 2for a segment the publisher calls 3, so the sequence disagrees with the publisher and with any other rendition bounded by the same timeline.- A later
trim(3)seesstate.offset (2) < 3and popspos3, dropping a segment that is still fetchable.Store each entry's publisher position so both the sequence and the retraction bound read it directly. The test helper at Lines 448-454 derives positions from
offset + entries.len(), so it reproduces the same assumption; add a case that pushes a duplicate-pts record at its real publisher position.🐛 Proposed fix (sketch)
- entries: VecDeque<Entry>, - /// The timeline position of `entries[0]`, and the playlist's `EXT-X-MEDIA-SEQUENCE` base. - /// Taken from the publisher's own positions, so it stays stable across reloads and agrees - /// with what the publisher says is still available. - offset: u64, + /// Each retained record with the publisher position it was indexed at, oldest first. + /// Positions are not consecutive: a skipped record leaves a gap. + entries: VecDeque<(u64, Entry)>, + /// The publisher position the next record would take, used as the `EXT-X-MEDIA-SEQUENCE` + /// base once the window is empty. + offset: u64,Then read the sequence from the front position, and bound the trim by position:
- Window { - sequence: self.offset, + Window { + sequence: self.entries.front().map_or(self.offset, |(position, _)| *position),- while state.offset < offset { - if state.entries.pop_front().is_none() { - state.offset = offset; - break; - } - state.offset += 1; - } + while state.entries.front().is_some_and(|(position, _)| *position < offset) { + state.entries.pop_front(); + } + // Keep the base aligned even when the window empties, so the next record lands at the + // publisher's media sequence. + state.offset = state.offset.max(offset);The duration-eviction loop then drops its
offset += 1, andpushrecords(position, entry).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-hls/src/export/segments.rs` around lines 186 - 244, Update the segment state to store each entry together with its publisher position, and have push record the supplied position when appending entries. In the eviction path, remove offset += 1 and derive the current sequence/offset from the retained front entry’s stored position; make trim compare and advance based on that stored position so skipped records are preserved. Update window and related accessors to use the front position directly, and revise the test helper plus add coverage for a duplicate-PTS record supplied at its actual publisher position.
🧹 Nitpick comments (6)
js/json/src/window.ts (2)
201-225: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
@publicand document theConsumerconstructor.The coding guidelines require
@publicon load-bearing classes.ProducerandConsumerare the package's primary surface and carry no@publictag.Producer's constructor has a doc comment at Line 92, butConsumer's constructor at Line 222 has none.📝 Proposed change
+ * `@public` */ export class Consumer<T> {+ /** Wrap a track subscriber to read a sliding record window from it. */ constructor(track: Moq.Track.Subscriber, config: ConsumerConfig = {}) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/json/src/window.ts` around lines 201 - 225, Add an `@public` annotation to the `Consumer` class and add a doc comment for its constructor, matching the existing `Producer` documentation style and describing the constructor’s parameters and behavior. Ensure the corresponding `Producer` class also carries the required `@public` annotation.Source: Coding guidelines
181-187: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReuse one
TextEncoderinstance.
#emitconstructs a newTextEncoderon every frame. Hoist it to a module-level constant, matching the single-instance pattern thatConsumer.#applyalso needs forTextDecoder.♻️ Proposed refactor
const MAX_GROUP_FRAMES = 256; + +const ENCODER = new TextEncoder(); +const DECODER = new TextDecoder();`#emit`(value: unknown): void { - const payload = new TextEncoder().encode(JSON.stringify(value)); + const payload = ENCODER.encode(JSON.stringify(value));- const parsed = JSON.parse(new TextDecoder().decode(payload)); + const parsed = JSON.parse(DECODER.decode(payload));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/json/src/window.ts` around lines 181 - 187, Update the window module to create a single module-level TextEncoder instance, then reuse it in Window.#emit instead of constructing a new encoder for each frame. Keep the existing JSON serialization and frame-writing behavior unchanged.js/json/src/window.test.ts (1)
112-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe compressed test does not cover a compressed trim op frame.
trim(15)leaves 5 records, so#trimmed(15) exceeds the window length (5).#needsSnapshotreturns true and the producer rolls a new snapshot group instead of writing a{"trim":15}frame. The compressed decode path for a trim op is therefore untested.Add a case that trims fewer records than remain, so the producer emits a compressed trim op inside an existing group.
💚 Proposed additional test
test("compressed roundtrip", async () => { const [producer, consumer] = pair(true); for (let n = 0; n < 20; n++) producer.append({ n }); producer.trim(15); producer.finish(); const held = replay(await drain(consumer)); expect(held.length).toBe(5); expect(held[0]).toEqual([15, { n: 15 }]); }); + + test("a compressed trim op stays in the open group", async () => { + const [producer, consumer] = pair(true); + for (let n = 0; n < 20; n++) producer.append({ n }); + // Fewer trims than records left, so no snapshot roll: the trim rides the group as an op. + producer.trim(5); + producer.finish(); + + const updates = await drain(consumer); + expect(updates.at(-1)).toEqual({ type: "trim", offset: 5 }); + expect(replay(updates).length).toBe(15); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/json/src/window.test.ts` around lines 112 - 121, Extend the “compressed roundtrip” test to include a trim that removes fewer records than remain in the window, ensuring `#needsSnapshot` stays false and a compressed trim op frame is emitted within the existing snapshot group. Keep the assertions verifying the decoded retained records and add coverage for the resulting post-trim sequence.drafts/draft-lcurley-moq-flate.md (2)
42-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply one Markdown style across all drafts.
drafts/draft-lcurley-moq-flate.md#L42-L42: convert the ATX headings reported at Lines 42, 46, 59, 75, 93, 103, 110, 120, and 126, or update the lint configuration.drafts/draft-lcurley-moq-hang.md#L300-L300: convert the ATX headings reported at Lines 300, 304, 328, and 346, and fix the fenced blocks at Lines 314 and 332.drafts/draft-lcurley-moq-json.md#L44-L44: convert the reported ATX headings and fix the fenced blocks at Lines 129, 146, and 153.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@drafts/draft-lcurley-moq-flate.md` at line 42, Apply one Markdown style across all three drafts: in drafts/draft-lcurley-moq-flate.md lines 42-42, convert the ATX headings at lines 42, 46, 59, 75, 93, 103, 110, 120, and 126; in drafts/draft-lcurley-moq-hang.md lines 300-300, convert the ATX headings at lines 300, 304, 328, and 346 and fix fenced blocks at lines 314 and 332; and in drafts/draft-lcurley-moq-json.md lines 44-44, convert the reported ATX headings and fix fenced blocks at lines 129, 146, and 153, or consistently update the lint configuration instead.Sources: Coding guidelines, Linters/SAST tools
1-129: 🗄️ Data Integrity & Integration | 🔵 TrivialRun the required draft and wire checks.
These changes define an on-the-wire format. Run
just drafts check. Runjust test smoke-fullfor the corresponding wire implementation changes before merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@drafts/draft-lcurley-moq-flate.md` around lines 1 - 129, Before merging these changes that define an on-the-wire format, run the draft validation checks using the command just drafts check to verify draft integrity, and then run the full smoke tests using just test smoke-full to validate that the corresponding wire format implementation works correctly with the new compression specification.Source: Coding guidelines
rs/moq-json/src/window.rs (1)
324-339: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider
#[non_exhaustive]onUpdate<T>.The module documents operation frames as forward-compatible: a frame carrying neither
appendnortrimis ignored. A future operation therefore adds anUpdatevariant, which breaks every exhaustive match downstream (moq_mux::timeline::Consumer::decodeandmoq_hls::export::rendition::watchboth match exhaustively today). Marking the enum#[non_exhaustive]now makes that addition non-breaking.moq_mux::timeline::Updatemirrors this shape and has the same exposure.Note the trade-off: downstream matches then need a wildcard arm and lose exhaustiveness checking, so decide once for both enums.
♻️ Proposed change
#[derive(Debug, Clone, PartialEq)] +#[non_exhaustive] pub enum Update<T> {Based on the coding guideline "Rust config structs with public fields must use
#[non_exhaustive]...; public enums that may gain variants must use#[non_exhaustive]".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-json/src/window.rs` around lines 324 - 339, Mark both public update enums, `moq_json::window::Update<T>` and the matching `moq_mux::timeline::Update`, with `#[non_exhaustive]`. Update exhaustive matches in `moq_mux::timeline::Consumer::decode` and `moq_hls::export::rendition::watch` to include wildcard handling while preserving existing behavior for known variants.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@drafts/draft-lcurley-moq-flate.md`:
- Line 129: Remove the AI attribution sentence from
drafts/draft-lcurley-moq-flate.md at lines 129-129 and
drafts/draft-lcurley-moq-json.md at lines 214-214, leaving the surrounding draft
content unchanged.
In `@drafts/draft-lcurley-moq-hang.md`:
- Around line 343-344: Clarify the consumer behavior for unindexed groups in the
recording/indexing specification: require consumers to fetch records unless an
explicit duration model makes PTS interpolation exact, rather than relying on
sequence contiguity alone. Define how the final record’s duration is determined
when no following record exists, and update the surrounding
duration/extrapolation rules accordingly.
- Around line 315-326: Define explicit numeric constraints for Timeline-related
values: require timescale to be a positive integer, restrict wall and pts to the
supported integer range, and ensure wall + pts remains lossless in JavaScript by
enforcing a safe-integer bound or adopting a lossless string/integer
representation. Document the rules for group and pts consistently with the
Timeline contract.
In `@drafts/draft-lcurley-moq-json.md`:
- Around line 144-159: Update Consumer.#applyOp to validate that each operation
frame contains exactly one of append or trim before changing window state;
reject frames containing both fields or neither field, while preserving existing
append and trim validation. Add regression tests covering both malformed cases
and confirming state is unchanged.
In `@js/hang/src/container/timeline.ts`:
- Around line 126-155: Update the timeline lifecycle around `#sweep`() and
finish() so finish() performs a final sweep before closing `#window`, ensuring
expired leading records are removed when recording stops. Document the resulting
wall-clock limitation: the window remains accurate only while groups are being
recorded, with end-of-stream cleanup handled by finish().
In `@js/json/src/window.ts`:
- Around line 294-307: Validate snapshot.offset at the start of `#applySnapshot`
and throw for missing, non-numeric, or otherwise invalid offsets before
assigning `#head` or `#tail` or iterating values, matching the strict
malformed-frame handling used by `#applyOp` for trim. Preserve the existing
snapshot adoption behavior for valid offsets.
In `@rs/moq-mux/src/timeline.rs`:
- Around line 205-227: Update timeline::write to map moq_json::Error::Window
into the non-fatal moq_net error variant expected by container::Producer::write
and fmp4::Import::extract, replacing the unreachable! panic path for rejected
trims. Preserve the existing transport-error mapping and keep unreachable
handling only for encoder errors that cannot occur.
---
Outside diff comments:
In `@rs/moq-hls/src/export/segments.rs`:
- Around line 186-244: Update the segment state to store each entry together
with its publisher position, and have push record the supplied position when
appending entries. In the eviction path, remove offset += 1 and derive the
current sequence/offset from the retained front entry’s stored position; make
trim compare and advance based on that stored position so skipped records are
preserved. Update window and related accessors to use the front position
directly, and revise the test helper plus add coverage for a duplicate-PTS
record supplied at its actual publisher position.
---
Nitpick comments:
In `@drafts/draft-lcurley-moq-flate.md`:
- Line 42: Apply one Markdown style across all three drafts: in
drafts/draft-lcurley-moq-flate.md lines 42-42, convert the ATX headings at lines
42, 46, 59, 75, 93, 103, 110, 120, and 126; in drafts/draft-lcurley-moq-hang.md
lines 300-300, convert the ATX headings at lines 300, 304, 328, and 346 and fix
fenced blocks at lines 314 and 332; and in drafts/draft-lcurley-moq-json.md
lines 44-44, convert the reported ATX headings and fix fenced blocks at lines
129, 146, and 153, or consistently update the lint configuration instead.
- Around line 1-129: Before merging these changes that define an on-the-wire
format, run the draft validation checks using the command just drafts check to
verify draft integrity, and then run the full smoke tests using just test
smoke-full to validate that the corresponding wire format implementation works
correctly with the new compression specification.
In `@js/json/src/window.test.ts`:
- Around line 112-121: Extend the “compressed roundtrip” test to include a trim
that removes fewer records than remain in the window, ensuring `#needsSnapshot`
stays false and a compressed trim op frame is emitted within the existing
snapshot group. Keep the assertions verifying the decoded retained records and
add coverage for the resulting post-trim sequence.
In `@js/json/src/window.ts`:
- Around line 201-225: Add an `@public` annotation to the `Consumer` class and
add a doc comment for its constructor, matching the existing `Producer`
documentation style and describing the constructor’s parameters and behavior.
Ensure the corresponding `Producer` class also carries the required `@public`
annotation.
- Around line 181-187: Update the window module to create a single module-level
TextEncoder instance, then reuse it in Window.#emit instead of constructing a
new encoder for each frame. Keep the existing JSON serialization and
frame-writing behavior unchanged.
In `@rs/moq-json/src/window.rs`:
- Around line 324-339: Mark both public update enums,
`moq_json::window::Update<T>` and the matching `moq_mux::timeline::Update`, with
`#[non_exhaustive]`. Update exhaustive matches in
`moq_mux::timeline::Consumer::decode` and `moq_hls::export::rendition::watch` to
include wildcard handling while preserving existing behavior for known variants.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 72e06a9c-6bf1-4cbd-a8ba-60ec7fb40d6a
📒 Files selected for processing (17)
drafts/draft-lcurley-moq-flate.mddrafts/draft-lcurley-moq-hang.mddrafts/draft-lcurley-moq-json.mdjs/hang/src/container/timeline.tsjs/json/src/index.tsjs/json/src/window.test.tsjs/json/src/window.tsrs/moq-hls/src/export/rendition.rsrs/moq-hls/src/export/segments.rsrs/moq-json/Cargo.tomlrs/moq-json/src/lib.rsrs/moq-json/src/window.rsrs/moq-mux/src/container/fmp4/import.rsrs/moq-mux/src/container/fmp4/import_test.rsrs/moq-mux/src/container/producer.rsrs/moq-mux/src/timeline.rsrs/moq-net/src/model/group.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5124212541
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 035a3bbe14
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fce4ee40cb
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 986107dc51
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2aa36b8b8b
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 629ed6cd1e
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@js/json/src/window.ts`:
- Line 123: Update the append logic around `#window.push` in the window
implementation to retain an immutable canonical value rather than the caller’s
object reference before publishing the first frame. Ensure every later `#snapshot`
uses that retained value, and add a regression test that mutates an appended
object before forcing a group roll and verifies the serialized position remains
unchanged.
- Around line 125-136: Update the snapshot/roll flow in the window publish and
trim paths so snapshot materialization completes before replacing or resetting
`#group`, `#frames`, and `#trimmed`. If a post-roll write fails, close and clear the
replacement group and restore window state so the next append creates a fresh
snapshot rather than emitting frame zero; prevent `#trimmed` from becoming
negative. Add a regression test covering serialization failure during a roll
followed by a successful append.
- Around line 120-122: Update the position boundary check in Producer to reject
position === MAX_POSITION by changing the comparison from greater-than to
greater-than-or-equal, matching Consumer.#applySnapshot and the Rust producer’s
boundary behavior.
In `@js/net/src/track.ts`:
- Around line 667-669: Remove the group.close() call from the stale-group branch
in the Consumer.next() flow, leaving stale groups discarded without mutating
shared state. Add a regression test in track.test.ts that uses two subscribers
and a late group, verifying the producer and other subscriber remain active.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f36c0e1-8d4a-47ab-9775-51865743ebee
📒 Files selected for processing (8)
js/json/src/window.test.tsjs/json/src/window.tsjs/net/src/track.tsrs/moq-hls/src/export/segments.rsrs/moq-json/src/window.rsrs/moq-mux/src/container/fmp4/import.rsrs/moq-mux/src/container/producer.rsrs/moq-mux/src/timeline.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- rs/moq-mux/src/container/fmp4/import.rs
- rs/moq-mux/src/container/producer.rs
- rs/moq-mux/src/timeline.rs
- rs/moq-hls/src/export/segments.rs
- rs/moq-json/src/window.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9cec3e5d4
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a9a84ef75
ℹ️ 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".
| // appending keeps the window from briefly listing content that is already gone. | ||
| this.#sweep(); | ||
|
|
||
| if (this.#lastPts !== undefined && pts < this.#lastPts + this.#granularityUs) return; |
There was a problem hiding this comment.
When #sweep() removes every published record, #published becomes empty but #lastPts still points at the retired record. If retention is shorter than granularity, or a publisher resumes after its only record aged out, the next group within that old granularity hits this return and no replacement record or timestamp is added, leaving the timeline window empty even though media is being produced. Reset #lastPts when the sweep drains the window, or only throttle while there is a live record to extend.
Useful? React with 👍 / 👎.
| Poll::Ready(Ok(None)) => { | ||
| self.group = None; | ||
| group_ended = true; |
There was a problem hiding this comment.
If the newest window group closes before frame 0 is read, for example because a peer sends an empty final group or a half-created group with no snapshot, this branch drops it and the outer loop can return Ok(None). That makes a malformed window look like a clean empty/end state to late joiners, even though each window group is supposed to start with a snapshot; return an Error::Window when self.frames == 0 here so consumers do not silently lose the advertised window.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1469be7e9
ℹ️ 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".
| let frame: Moq.Group.Frame | undefined; | ||
| try { | ||
| frame = await this.#group.readFrame(); |
There was a problem hiding this comment.
When a consumer already has #group, this await only observes that group's frames/close; tryNextGroup() is not called again until #group is cleared. Over MoQ/WebTransport, the next self-contained window group can arrive before the previous group's FIN, so the subscriber can sit on an idle old group and ignore the fresh snapshot indefinitely, or until an obsolete frame/FIN arrives. Race the current group with track group arrival and drain to the newest group before blocking.
Useful? React with 👍 / 👎.
| // Drain everything already buffered before blocking, so a late joiner catches up in one step. | ||
| let advanced = false; | ||
| for (let frame = this.#group.tryReadFrame(); frame !== undefined; frame = this.#group.tryReadFrame()) { | ||
| this.#apply(frame.payload); |
There was a problem hiding this comment.
If the latest window group has evicted frame 0 under byte pressure, Group.Consumer.skipped is true and readFrame() would throw Lagged, which the catch below resyncs from. This fast path bypasses that check: tryReadFrame() returns the first remaining operation and #apply treats it as a snapshot, turning a disposable cache miss into a fatal malformed-snapshot error. Check #group.skipped before draining or use the throwing read path.
Useful? React with 👍 / 👎.
| if entry.pts < back_pts { | ||
| tracing::warn!("timeline jumped backwards; resetting the playlist window"); | ||
| state.dropped += state.entries.len() as u64; | ||
| // Everything listed leaves the playlist, so the media sequence counts them all. | ||
| state.sequence += state.entries.len() as u64; | ||
| state.entries.clear(); |
There was a problem hiding this comment.
When a live timeline jumps backward, the last entry in state.entries is the live edge and has never been listed as an HLS segment unless ended is already true. Adding the full entry count here creates the same phantom media sequence fixed for trims: after entries 0 and 1 were advertised as only MSN 0, a reset makes the next new segment start at MSN 2, so clients think MSN 1 was skipped. Advance by the number of listed segments instead.
Useful? React with 👍 / 👎.
| It MUST be a non-negative integer, and a receiver MUST reject a snapshot whose offset is absent or not one: every position derives from it, so a bad offset silently corrupts them all rather than failing. | ||
| It MUST NOT exceed 2^53-1, so a receiver using IEEE 754 doubles (a JSON parser's usual number type) counts exactly. |
There was a problem hiding this comment.
The draft only caps offset, but the Rust and JS consumers now reject a snapshot whose offset + values.length crosses the exact-integer limit, for example offset = 2^53 - 2 with two records. As written, another implementation can be conforming while producing a snapshot this repo rejects; state the same tail/append bound here so the normative wire spec matches the implementations. CLAUDE.mdL168-L168
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2422229acb
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bafc419a42
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab75852cfb
ℹ️ 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".
| if let Some(update) = self.pending.pop_front() { | ||
| return Poll::Ready(Ok(Some(update))); |
There was a problem hiding this comment.
When frame 0 of a rolled group snapshots a large window, apply_snapshot queues all of its records in pending. After the first queued record is returned, if a newer group has already arrived with the remaining positions trimmed, this branch still drains the stale queue before polling the track again, so a slow timeline/HLS consumer can emit records from a group the window protocol says to abandon. Poll for newer groups before returning queued appends, and clear/rebuild the queue when switching snapshots.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one, and leaving the thread open so a human can overrule me.
The observation is right: draining the queue can emit records a newer snapshot has already trimmed. But it converges. Those records were genuinely in the window when the snapshot was written, and on switching, report_trim emits a Trim to the newer head that retracts exactly them, so the update log is a truthful history and the consumer ends up holding the live window. Staleness is inherent to an async consumer regardless: anything yielded can be trimmed a microsecond later.
The suggested remedy is where it breaks. push_append advances next when it queues a record, not when it yields one (/// Deserialize a record and queue it, advancing the yield cursor past it). Clearing the queue on a snapshot switch would leave next past positions that were never delivered, and the new snapshot's if position < self.next { continue } filter would then skip them permanently. That is silent record loss, strictly worse than the transient staleness it removes. Doing it safely would mean tracking yielded separately from queued, which is a real change to the consumer's bookkeeping rather than a reordering of this branch.
Added a_slow_consumer_converges_after_draining_a_stale_snapshot in bfda2a2 to pin the property instead: it joins late so frame 0 queues a whole window, rolls a new group behind the consumer's back with the head trimmed, then asserts no position repeats, none is lost, and the replayed result is exactly the live window.
🤖 Addressed by Claude Code
(written by Opus 5)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfda2a2515
ℹ️ 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".
| } | ||
| false => (Bytes::from(payload), None), | ||
| }; | ||
| group.write_frame(moq_net::Timestamp::now(), slice)?; |
There was a problem hiding this comment.
When write_frame fails after append_group() has already published the new group, this ? exits without finishing or aborting that half-created group. This is reachable with the same post-append_group failures the existing snapshot/stream producers defend against, such as an extreme track timescale that makes Timestamp::now() unconvertible or an oversized snapshot frame; a subscriber that advances into the published group then waits on a group with no snapshot and no end, while the producer has already rolled its local state back.
Useful? React with 👍 / 👎.
| # Acknowledgments | ||
| {:numbered="false"} | ||
|
|
||
| This document was drafted with the assistance of Claude, an AI assistant by Anthropic. |
There was a problem hiding this comment.
This new draft is rendered into the /draft docs and is also the source submitted to the datatracker, so this source marker violates the repository rule that code comments and doc pages must not carry AI attribution; the same acknowledgment was added to draft-lcurley-moq-json.md, so remove both. CLAUDE.mdL89-L89
Useful? React with 👍 / 👎.
bfda2a2 to
c62241b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c62241b4ce
ℹ️ 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".
| if feed.history.is_empty() { | ||
| feed.anchor = None; | ||
| } |
There was a problem hiding this comment.
When a retraction removes every history entry and the catalog does not declare a wall clock, clearing feed.anchor makes the next push estimate a new pts-zero time from a different arrival timestamp. The resulting dynamic MPD changes availabilityStartTime across reloads even though the timeline did not restart, which can shift the presentation for active DASH clients. Keep the anchor until the actual backward-jump reset path. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
| while self.held.front().is_some_and(|&(position, _)| position < offset) { | ||
| let (_, segment) = self.held.pop_front().expect("peeked above"); | ||
| self.trimmed = segment + 1; |
There was a problem hiding this comment.
When an untrusted timeline publishes a record with segment = u64::MAX and then trims that record, this addition overflows. Development builds abort, while release builds wrap self.trimmed to 0, causing the downstream HLS trim to retain rows whose media was retracted. Validate the decoded segment range or use checked arithmetic before updating the cursor. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
c62241b to
095519a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 095519a07b
ℹ️ 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".
| while state.rows.front().is_some_and(|row| row.segment < segment) { | ||
| state.rows.pop_front(); |
There was a problem hiding this comment.
When a retraction removes every row and the timeline subsequently ends, State::window() derives the media sequence from the now-empty deque and reports 0. A client that previously saw a positive EXT-X-MEDIA-SEQUENCE can therefore receive a final empty ENDLIST whose sequence moves backward, contrary to the monotonic behavior this method promises. Retain the retracted head or next segment number even while the row deque is empty.
Useful? React with 👍 / 👎.
| this.#head = snapshot.offset; | ||
| this.#tail = snapshot.offset + values.length; |
There was a problem hiding this comment.
When a buggy or hostile publisher sends a newer snapshot whose offset + values.length is below #next, these assignments rewind #tail while leaving the consumed cursor unchanged. For example, after positions 0 through 9 were delivered, a snapshot containing only position 0 makes subsequent appends start again at position 1 and silently skips them until the tail reaches 10. Reject a snapshot whose tail precedes #next so repeated positions cannot turn into silent data loss.
Useful? React with 👍 / 👎.
kixelated
changed the title
The timeline was an append-only `moq_json::stream`, so it listed every
segment ever published, including long-evicted ones. An HLS playlist
built from it advertised segments that could no longer be fetched.
Root cause of why removal was impossible: a stream's whole log rides one
group that is never rolled, and a consumer always starts at frame 0.
Once moq-net evicted the earliest frames the track was bricked for late
joiners (`Error::Lagged`), and with compression the retained suffix is
undecodable anyway since its DEFLATE window depends on the evicted
prefix. There was no shape in which a record could be dropped.
Adds `moq_json::window`: each group is self-contained (snapshot at frame
0, then `{"append":V}` / `{"trim":N}` operations) and rolls once the
records trimmed since the snapshot outnumber those still in the window.
That bounds total bytes at roughly 2x an append-only log and lets old
groups be dropped freely, since a late joiner reads only the newest.
Trims are driven by the cache itself, via the new
`group::Consumer::closed()`. Both moq-net eviction paths abort the group
and abort closes the shared state. A consumer handle pins no frames, so
the timeline observes availability without extending it, and no task,
timer, or callback is involved. The sweep runs as segments are recorded,
which is also the only time the publisher's cache shrinks.
A segment spans group ranges on every enrolled track, so its record is
retracted once any group it covers is gone: a consumer fetches the whole
segment, and a hole anywhere in it breaks the segment.
moq-hls now treats a retraction as the hard bound on the playlist, with
the configured duration window staying a soft preference; whichever
removes more wins.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
095519a to
16d54f8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16d54f87f0
ℹ️ 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".
| /// assembly does, i.e. when no route can serve it again. | ||
| pub fn is_closed(&self) -> bool { | ||
| match &self.inner { | ||
| ConsumerKind::Plain(plain) => plain.state.is_closed(), |
There was a problem hiding this comment.
When a media group exceeds the per-group cache cap, GroupState::evict drops its leading frames without closing the state, so this returns false even though fetching the group from frame 0 now returns Lagged. The timeline's new sweep consequently publishes or retains an unfetchable segment. The mirrored JS path has the same gap because appendFrame advances offset at 1,024 frames or 32 MiB without settling gone, while #sweep checks only isGone. Expose complete-group availability from the group layer, including head-frame eviction, and use it in both timeline sweep implementations.
AGENTS.md reference: AGENTS.md:L100-L103
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| /// Finish the track, closing any open group. | ||
| pub fn finish(&mut self) -> Result<()> { |
There was a problem hiding this comment.
This newly public terminal operation leaves the same handle usable, so producer.finish(); producer.append(...) remains expressible and fails only at runtime after the shared track has closed. Make finish consume self, and reconsider the clonable shared terminal state if necessary, so ordinary use-after-finish is rejected by the type system before this API is published.
AGENTS.md reference: AGENTS.md:L126-L131
Useful? React with 👍 / 👎.
| }; | ||
|
|
||
| self.head = snapshot.offset; | ||
| self.tail = tail; |
There was a problem hiding this comment.
Fresh evidence beyond the existing JS finding is that the Rust snapshot path independently overwrites self.tail without comparing it with self.next. After positions 0 through 9 have been delivered, a newer snapshot with tail 1 rewinds the operation cursor; subsequent appends reuse positions 1 through 9 and are silently discarded until the tail catches up, violating the protocol's never-repeated-position invariant. Reject snapshots whose computed tail is below self.next.
Useful? React with 👍 / 👎.
`Fanout::trim` cleared `Feed::anchor` whenever a retraction emptied the replay history. The anchor maps the pts axis onto wall clock, and a retraction says nothing about that axis: only a timeline restart (the backwards-jump path in `push`) invalidates it. Clearing it made the next record re-estimate from a fresh `now()`, jumping `EXT-X-PROGRAM-DATE-TIME` and DASH `availabilityStartTime` for every client following unchanged content. The correlation makes it worse than a random jump: a retraction deep enough to empty the history means the publisher is shedding media, which is exactly when the anchor's "this segment just ended" assumption is at its least true. Covered by two tests: one asserting a trim past the whole window keeps the anchor, and its contrast case asserting a backwards pts jump still re-anchors. The first fails without this change. Also restores the doc comment on `takeover_splices_mid_group`, which the new `spliced_group_stays_available_across_a_takeover` test was inserted in front of, leaving one test carrying both comments and the other none. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…sumer
`group::Consumer::{closed, poll_closed, is_closed}` answered two different
questions depending on which variant the consumer happened to be, with the
same name and the same doc comment:
- `Plain` read the group's own state, so it meant "the publisher's cache
still holds these frames" -- a real availability guarantee.
- `Spliced` read `resume::Group.state`, which is the whole logical *track's*
routing state (segments, epoch, finished, abort), never the per-route
`Current.group` that actually holds frames. So it meant "this track's
routing table has not been torn down".
The `Spliced` answer is therefore not group-scoped at all (every spliced
group on a track answers identically), and it stays false for as long as the
track keeps publishing. An index track built on a consumer from
`track::Subscriber::recv_group()` would simply never retract, which is the
bug the timeline exists to prevent. Monotonicity does not save it: the
failure is a signal that never fires, and a permanently-false predicate is
perfectly monotone.
Replace it with `group::Availability`, minted by
`group::Producer::availability()`. Only the side that owns a cache can answer
the question, so making the producer the only source removes the case that
had no honest answer rather than answering it badly. A relay mints one too,
since it creates a producer per group it caches.
The token is also the right weight for the job: a `kio::Consumer<GroupState>`
rather than a `group::Consumer` carrying a read cursor and an inline 8-slot
frame prefetch it has no use for. The timeline holds one per group, per
track, per record, for the whole window.
Names it `is_gone`/`gone`/`poll_gone` to match js/net's `isGone`/`gone`,
which retires the Rust-vs-JS naming divergence this PR previously had to
document. The remaining asymmetry is only where the handle is minted; js/net
has no spliced groups, so its consumer-side signal is unambiguous.
`resume::Group::{is_closed, poll_closed}` go with it, having existed only to
serve the `Spliced` arm.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49365e659e
ℹ️ 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".
| self.drop_sink(); | ||
| return; | ||
| } | ||
| self.indexed.push_back(covered); |
There was a problem hiding this comment.
When one enrolled track races ahead, or a reservation delays flushing, one of its pending groups can be evicted before another track completes the segment. The leading pump() sweep cannot see that group because its availability handle is still in TrackState::pending; emit() then appends the record and only afterward stores the already-gone handle here, with no further sweep until another report or finish(). The timeline can therefore advertise an HLS segment that was already unfetchable when indexed. Fresh evidence beyond the prior final-sweep finding is that this loss occurs before the record enters indexed; the mirrored ordering at js/hang/src/container/timeline.ts:468-469 has the same gap.
Useful? React with 👍 / 👎.
`Availability` held a `kio::Consumer<GroupState>`, which registers as a consumer. A watcher is not a reader: it never calls `read_frame`, and counting it as one holds `group::Producer::unused()` open for as long as the index track keeps watching. No production code awaits group-level `unused` today, so this was latent rather than live, but it is the exact trap `track::Demand` was already shaped to avoid. Switch to `kio::ProducerWeak<GroupState>`, the same handle `Demand` uses, which holds no ref count and so keeps neither the group open nor its frames pinned. It does keep the state allocated, which is what still lets `gone()` report the abort cause after the channel closed. This makes the two watch-only handles in moq-net consistent: `track::Demand` watches subscriber interest, `group::Availability` watches cache residency, and both are weak, cloneable, and minted from their producer. `availability_is_not_a_consumer` pins it; reverting to `Consumer` fails it on `a watcher must leave the group unused`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b61b5ae8c0
ℹ️ 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".
| /// Write one operation frame into the open group. | ||
| fn write(&mut self, payload: Vec<u8>) -> Result<()> { | ||
| let slice = match self.encoder.as_mut() { | ||
| Some(encoder) => encoder.frame(&payload), |
There was a problem hiding this comment.
When compression is enabled and write_frame rejects an operation after encoder.frame succeeds, such as a high-entropy append whose compressed payload exceeds the 32 MiB group limit, the encoder has already advanced even though append rolls the record back. If the caller catches the error and publishes a smaller operation, it reuses this group and emits bytes that depend on the missing frame, so the consumer's DEFLATE context can no longer decode the window. Fresh evidence beyond the earlier rollback finding is that the rollback restores the logical window but not this compression state; abort or roll the group on write failure. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L89-L89
Useful? React with 👍 / 👎.
| // Same rollback as `append`: an unpublished trim must not move the head. | ||
| this.#offset -= count; | ||
| this.#trimmed -= count; | ||
| this.#window.unshift(...removed); |
There was a problem hiding this comment.
When a large window trim fails to publish, this spread passes every removed record as a separate unshift argument and can exceed the browser engine's argument limit. The rollback then throws RangeError after restoring #offset and #trimmed but before restoring #window, leaving the producer internally corrupted. Fresh evidence beyond the earlier rollback finding is that the restoration itself fails for sufficiently large windows; restore with iteration, concatenation, or bounded chunks instead. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L89-L89
Useful? React with 👍 / 👎.
`Availability` was a second watch-only handle sitting next to a family that already existed. `broadcast::Demand` and `track::Demand` are both cloneable, weak, producer-minted handles that report demand *and* lifetime: `track::Demand` already pairs `used`/`unused` with `closed`. Availability was the lifetime half of that same shape under a different name, which would have left `group::Demand` to be added separately later for the demand half. So there is one handle per level now. `group::Producer::demand()` returns a `group::Demand` exposing `is_used`/`used`/`unused` (plus `poll_` forms) for reader interest, and `is_closed`/`closed`/`poll_closed` for whether the group still exists. `group::Producer::unused()` already existed, so the demand half is a real question at this level rather than padding. For a group, closure *is* the availability signal: a group that finished cleanly stays open while its frames are still retrievable and closes once they aren't. The doc says so where it can't be missed, since keying an index track off a clean finish instead would retract everything the moment it published. That also lines up with `broadcast::Consumer`, which already pairs `is_closed` with `is_finished`. No behavior change: the timeline reads `is_closed()` where it read `is_gone()`, over the same weak handle. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Summary
moq_json::stream, so it listed every segment ever published, including long-evicted ones. An HLS playlist built from it advertised segments that could no longer be fetched, and moq-hls papered over the misses by emittingEXT-X-DISCONTINUITYafter a failed fetch.Error::Lagged), and with compression the retained suffix is undecodable anyway since its DEFLATE window depends on the evicted prefix. There was no shape in which a record could be dropped.moq_json::window: each group is self-contained (snapshot at frame 0, then{"append":V}/{"trim":N}operations) and rolls once the records trimmed since the snapshot outnumber those still in the window. That bounds total bytes at roughly 2x an append-only log and lets old groups be dropped freely, since a late joiner reads only the newest.group::Consumer::closed(). Both moq-net eviction paths abort the group (Error::Evictedunder pool pressure,Error::Oldby age) and abort closes the shared state. A consumer handle pins no frames, so the timeline observes availability without extending it, and no task, timer, or callback is involved. The sweep runs as segments are recorded, which is also the only time the publisher's cache shrinks.Design notes for reviewers
Three behaviors that are easy to get backwards, all pinned by tests:
group::Demand::is_closed()is availability, not completion. A cleanly finished group stays open while it is still cached and fetchable; only the abort closes it. An index track keying off a clean finish would retract everything immediately. This matchesbroadcast::Consumer, which already pairsis_closedwithis_finished. JS spells the same conceptGroup.Consumer.gone/isGone, becauseclosedthere already means completion (it settlesnullon a clean finish).Update::Trimonly ever retracts records the consumer was told about. The first snapshot adopts its offset silently, so a consumer joining a window that already trimmed gets no trim for records it never held; a gap surfaces instead as a jump in the first append's position. Note the corollary, which cost me two wrong tests: a late joiner does legitimately see a trim when the window has not rolled, because its snapshot included the record being retracted.Branch targeting
Targets
dev, for a signature change to an exported symbol:@moq/hang'sContainer.Timeline.Recorder.recordnow takes theMoq.Group.Producerrather than its sequence number, since the timeline has to hold a handle on the group to watch it leave the cache. Taking the group also makes the sequence impossible to mismatch with the group being watched.This PR was originally written against
main, before #2547 reworked the timeline into a per-broadcast segment index. It has been rebased ontodevand the integration rewritten against that model rather than ported; the diff against the oldmain-based branch is not meaningful. Building it against dev's real timeline is what validated the primitive, and it changed three things (see below).Public API changes
Added
rs/moq-net:group::Demandandgroup::Producer::demand()that mints it, completing thebroadcast::Demand/track::Demandfamily. Exposesis_used/used/unused(pluspoll_forms) for reader interest, andis_closed/closed/poll_closedfor whether the group still exists.rs/moq-json: thewindowmodule (Producer,Consumer,Update,ProducerConfig,ConsumerConfig), plus anError::Windowvariant (the enum is#[non_exhaustive]).js/json: theWindownamespace, mirroring the above.js/net:Group.Consumer.{gone, isGone},Group.Producer.evict()(@internal),Track.Subscriber.tryNextGroup().rs/moq-mux:timeline::Update.Changed
js/hang:Container.Timeline.Recorder.recordsignature, as above. This is thedevtrigger.rs/moq-mux:timeline::Consumer::{next, poll_next}now yieldUpdate<E>rather thanEntry<E>, since an entry alone cannot express a retraction.Entryis unchanged and rides insideUpdate::Append.Recorder::recordalso takes the group, but it ispub(crate).Wire format (breaking). The timeline track changes from an append-only stream to a window. Nothing negotiates this: a new consumer against an old publisher errors rather than mis-parsing. That is acceptable here specifically because the catalog's
timelinefield was never specified — this PR is what specifies it, so breaking it now costs nothing and breaking it later would cost a version.What rebasing onto
devchangedWorth reading, since these are the parts that differ from the
mainversion by more than mechanics:group::Consumeris aPlain/Splicedenum (from the route-splicing work). OnlyPlainhas a publisher whose cache can drop it; aSplicedgroup is a reader-side reassembly across routes, with no single publisher. Putting the signal onConsumertherefore gave it two different meanings under one name, and theSplicedone was not group-scoped at all: it read the whole logical track's routing state, so it stayed false for as long as the track kept publishing. An index track built on a subscriber's group would simply never retract. The signal moved togroup::Demand, minted only bygroup::Producer::demand(), because only the side that owns a cache can answer the question. That removes the case with no honest answer instead of answering it badly. It also completes an existing family rather than inventing a parallel one:broadcast::Demandandtrack::Demandare already cloneable, weak, producer-minted handles reporting demand and lifetime, andtrack::Demandalready pairsused/unusedwithclosed. Being weak (kio::ProducerWeak, the same handletrack::Demanduses) matters twice over: the per-record cost drops to a pointer instead of agroup::Consumercarrying a read cursor and an inline 8-slot frame prefetch, and a watcher no longer registers as a reader, which would have heldgroup::Producer::unused()open for as long as an index track kept watching.positionbecame redundant, so it is gone from the timeline's API. dev'sRecord.segmentalready exists and already anchorsEXT-X-MEDIA-SEQUENCE.timeline::Updatetherefore speaks only segment numbers and never leaks the window's coordinate system; the consumer translates positions to segments internally, holding the pair rather than assuming the two coincide.Cross-Package Sync
rs/hangcatalog/container ->js/hang,drafts/: done.draft-lcurley-moq-hang's Timeline section now specifies the window framing and the retraction rules.draft-lcurley-moq-json(snapshot / stream / window) anddraft-lcurley-moq-flate(group compression, factored out since it is not JSON-specific). Cross-references are manual reference entries rather thanI-D.anchors, since neither is on the datatracker yet; those become plain anchors once submitted.doc/: no change. The timeline is not documented on the site, and no CLI surface moved.Cross-language shape
Rust and JS now agree on the word:
Availability::{is_gone, gone}againstGroup.Consumer.{isGone, gone}. The earlieris_closedvsgonedivergence is gone with it.What still differs is where the handle is minted: Rust from the producer, JS from a consumer. That asymmetry is deliberate rather than an oversight.
js/nethas no spliced groups, so its consumer-side signal has exactly one meaning and answers about the local cache in both the publish and subscribe directions. Mirroring the Rust shape there would be churn with no correctness payoff in an already-large diff.Review follow-ups
Two defects found reviewing the branch, fixed in
6427611ca:moq-hlsdropped the wall-clock anchor on a retraction.Fanout::trimclearedFeed::anchorwhenever a trim emptied the replay history. The anchor maps the pts axis onto wall clock, and a retraction says nothing about that axis; only a timeline restart invalidates it. The next record then re-estimated from a freshnow(), jumpingEXT-X-PROGRAM-DATE-TIMEand DASHavailabilityStartTimeover unchanged content. The correlation makes it worse than a random jump: a retraction deep enough to empty the history means the publisher is shedding media, which is exactly when the anchor's "this segment just ended" assumption is least true. Two tests cover it, and the positive one was mutation-checked (it fails without the fix).takeover_splices_mid_groupundocumented and the new spliced test carrying two comments.Test plan
just checkpasses.just rs cipasses, includingcargo docwith-D warnings.just js checkandbun testpass.just drafts check(kramdown-rfc) passes on all 10 drafts. (TheHT characterswarning on the hang draft is pre-existing ondev: 32 tab-containing lines in its schema blocks, none of them touched here.)group::Consumerclosing on eviction vs. surviving a clean finish; window append/trim/roll, lossless group switching, gap-as-position-jump, compression, and an unknown forward-compatible operation; timeline retraction on eviction, a segment going when any track loses a group, a finished group not counting as a retraction, out-of-order eviction dropping the records in front, the final sweep infinish, a retraction that empties the window, and a fold showing the updates converge on exactly the available segments; moq-hls trims dropping unfetchable segments, never rewinding the media sequence, surviving a trim past the window, and leaving the duration bound in force.js/json/src/window.test.ts(two decode hand-written wire frames, so the decoder is verified against the byte shape rather than only against its own producer), plus 4 timeline retraction tests mirroring the Rust ones.(Written by Opus 5)