Repository navigation
fix(net): preserve IETF timestamps across negotiated lite hops - #6300
Conversation
Quest-Claim: a1f396c1fad49551
Co-Authored-By: GPT-6 <noreply@openai.com>
kixelated
changed the title
…on/serve-timed # Conflicts: # drafts/draft-lcurley-moq-lite.md # rs/moq-net/src/model/track.rs
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe changes replace the internal unchecked-track flag with IETF version provenance in JavaScript and Rust. MoQ Lite serving now uses IETF extension negotiation to determine whether it includes track timescales and timestamps. Protocol text and tests describe preservation across negotiated hops, untimed listings, and untimed service when peers do not negotiate the extension. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 10 files. (6 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title names the main change: timestamps from IETF tracks are preserved across moq-lite hops when the ietf extension is negotiated. It matches the changeset, which replaces the unchecked flag with … Description check The description states the problem (timed moq-transport tracks lose timestamps at the first moq-lite hop), the approach (Info::ietf() as sole provenance, negotiated-only timescale preservation, JS mir… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title names the main change: timestamps from IETF tracks are preserved across moq-lite hops when the ietf extension is negotiated. It matches the changeset, which replaces the unchecked flag with Info.ietf and makes lite_timescale depend on negotiation. The title follows the fix(net) convention and is 62 characters.
Full details: Description check
-
Explanation
The description states the problem (timed moq-transport tracks lose timestamps at the first moq-lite hop), the approach (Info::ietf() as sole provenance, negotiated-only timescale preservation, JS mirror with Info.ietf replacing Info.unchecked), validation, and impact. It matches the raw summary's changes across Rust, JS, drafts, and the quest docs.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 10 files. (6 skipped: 4 unsupported, 2 too large.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ Finishing Touches
-
✨ Simplify code
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
🔧 Clippy (1.99.0)
-
Clippy execution timed out
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 help to get the list of available commands.
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. |
![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: f83ee181e8
ℹ️ 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".
| Some(crate::broadcast::TracksEvent::Added { name, info }) => lite::TracksEvent::Added { | ||
| name: name.to_string().into(), | ||
| info: lite::TrackInfo::serve(&info, shared.version), | ||
| info: lite::TrackInfo::serve(&info, shared.version, ietf), |
There was a problem hiding this comment.
When an IETF track listing crosses a negotiated ietf hop, this now forwards its timescale, but TRACKS_ADD carries no TRACK_PROPERTIES; the receiver therefore stores the listed Info with properties: None. If that relay forwards the listing over a second, unextended hop, lite_timescale(false) mistakes it for a native-lite track and advertises the timescale again, while the subsequent Track/SUBSCRIBE path correctly serves the media untimed. A client may skip the Track Stream based on TRACKS_ADD, as the base protocol permits when it did not negotiate the extension, and then parse the untimed group framing as timestamped. The listing representation must retain unchecked provenance across relays or avoid advertising this timescale where provenance cannot accompany it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid: the receiving relay stored the listed Info without provenance and would re-list it as timed native lite. Fixed in e36ee4a: TRACKS_ADD now lists a moq-transport track untimed even to an extension peer (the Track Stream stays authoritative, and the extension already forbids skipping it). The hop test now asserts the listing is untimed for every offer and fails if the listing advertises the timescale; the ietf draft says the same.
(Written by Claude Opus 5.5)
TRACKS_ADD carries no TRACK_PROPERTIES, so a relay receiving the listing loses the unchecked provenance and would re-list the track as timed native lite to an unextended peer, whose media is served untimed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Resolve the moq-lite draft conflict by keeping main's 2^61 timestamp bound beside this branch's negotiated-extension exception to the frame 0 ordering rule, and both changelog entries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem
A timed moq-transport track loses its timestamps at the first moq-lite relay hop, even when both peers negotiate
ietf. A separateuncheckedflag also duplicates the track's already-carried ingress version, so the two can disagree after a hop.Approach
Use the original IETF version (
Info::ietf()) as the sole unchecked-timestamp provenance. Preserve that track's timescale only for a negotiated extension peer, consistently across TRACK_INFO, SUBSCRIBE, FETCH, and datagrams. A non-extension peer still receives it untimed. Listings (TRACKS_ADD) stay untimed even to an extension peer: they carry no TRACK_PROPERTIES, so a relay would lose the provenance and re-list the track as timed native lite; the extension already requires the Track Stream for listed tracks. Derive the receiver's rewind policy from the same provenance, so an IETF arrival-time rewind does not become a native-lite violation after a hop.Mirror the private ingress-version metadata in JS (
Info.ietfreplacesInfo.unchecked). JS has noietfpassthrough implementation, an explicit epic non-goal, so its lite egress stays untimed. Synchronize the timing explanation indoc/concept/moq-lite.mdand both matching drafts.Merged main to keep the public-ingest and priority refactors (moq-dev/moq#6111, moq-dev/moq#6142); a resume test from main that set
unchecked: truenow sets moq-transport provenance instead. Retires the finishedquest/m1/ietf-extension/serve-timed.md; its epic README now holds only the nightly moxygen work.Impact
track::Info::uncheckedand JS's@internalInfo.uncheckedare replaced by the existing private ingress provenance.lite_timescale(crate-private /@internal) now takes whether the hop negotiatedietf.ietfpeers now retain a relayed track's timescale and its existing timestamp prefixes. Unextended and published (lite-05/06) peers keep their previous untimed behavior. The lite draft relaxes the frame-0 ordering MUST and the "serve untimed" MUST only where a negotiated extension identifies unchecked provenance; the ietf extension draft states that it does, and that a listing stays untimed.Validation
Noneinstead ofSome(42ms)across the negotiated hop and passes after the change.cargo nextest run -p moq-net --all-features3028 pass (11 skips);js/netbun test2635 pass.just check: js, drafts, rs, quest, and drill stages pass; the flake stage stops at the unchanged Nix runner-image build because this host deniesunsharewriting/proc/self/uid_map(host limitation, not the branch).just test interop --all: all 127 checks pass across rust, python, go, cpp, js, js-native-node, js-native-bun, c, and gst, including delivery, auth refusals, browser close code, CLI FETCH, finite tails, compression, and drop coverage.quest check --base origin/main --branch quest/m1/ietf-extension/serve-timedpasses.Alternatives
Keeping two independent provenance flags permits disagreement after a hop. Sending timestamps to an unextended peer would subject unchecked arrival times to native-lite checks. Adding JS extension support would expand this beyond its epic's boundary.
Follow-ups
None new. The
ietf-extensionepic keeps the nightly moxygen conformance job.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code