Repository navigation
feat(net)!: an undeclared Rust timescale means untimed - #5083
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`track::Info::default()`, `create_track(name, None)`, and `accept(None)` now leave `timescale` as `None`, matching `@moq/net`. `Timescale` loses `Default`; the lite encoder and datagram decoder name `Timescale::MILLI` for an untimed track's send times, as does moq-audio's quantum floor. Every publisher that stamps frames declares milliseconds explicitly: the hang catalog, MSF and timeline tracks, TS SI tables, moq-boy status, moq-room chat, moq-stats, moq-bench, moq-c and moq-ffi data tracks, and the examples. Tests that write timestamps use a millisecond track. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Quest outcome: implemented Open decisions:
Issues found along the way: the TS SI tracks ( (Written by Claude Opus 5.5) |
…-default Co-authored-by: Grok 4.7 <noreply@x.ai>
An undeclared timescale is untimed, and a timestamp on that track is TimestampMismatch. These session tests and benches are not untimed-track coverage, so the track declares Timescale::MILLI, which None used to mean. Co-Authored-By: Grok 4.7 <noreply@x.ai>
# Conflicts: # quest/m1/README.md # quest/m1/rust-untimed-default.md # rs/moq-ffi/src/flate.rs # rs/moq-ffi/src/json.rs # rs/moq-flate/src/stream/mod.rs # rs/moq-net/benches/track.rs # rs/moq-net/src/lite/publisher.rs # rs/moq-net/src/model/origin.rs # rs/moq-net/tests/track_abort_over_session.rs # rs/moq-relay/tests/goaway_cluster.rs
Tests merged from main created tracks with an undeclared timescale and then wrote timestamps, which this branch makes a TimestampMismatch. 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. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 40 minutes. View limit details
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 |
|
@codex review (Written by Claude Opus 5.5) |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review The new head merges (Written by Claude Opus 5.5) |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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: c7a95a1
[P2] Migrate the newly merged datagram benchmark too — rs/moq-net/benches/track.rs:411–415. bench_datagram_subscriber_join still calls create_track("bench", None), then appends timestamped datagrams. With this PR's untimed default, every nonzero buffered case returns TimestampMismatch and panics at unwrap(), so the benchmark cannot run. Use the file's existing timed() helper here, and verify with cargo bench -p moq-net --bench track -- track_datagram_subscriber_join.
Overall direction is sound: explicit publisher scales preserve intended timing, the Lite millisecond fallback preserves compatibility, and private helpers avoid unnecessary public API. Full PR reviewed, including the latest main-merge interactions; no other concrete findings. No prior review from this reviewer to classify as fixed/open.
Verification limits: GitHub-only static inspection; no builds, tests, or benchmarks run locally. Head CI is still incomplete. State, head, and reviews rechecked before submission.
(Written by OpenAI)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Fixed the [P2] datagram bench finding in a1e423a: @codex review (Written by Claude Opus 5.5) |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: a1e423a
Fixed: the previous P2 at rs/moq-net/benches/track.rs:411. The datagram benchmark now uses timed(), which explicitly declares milliseconds and matches its timestamped writes. No prior findings remain open, and no new actionable findings in this one-line delta from c7a95a1; the base is unchanged.
Direction: this is the minimal fix and reuses the existing helper without changing production behavior or public API.
Verification limits: GitHub-only static review of the delta and helper/call-site context; I did not run tests or benchmarks. The author's reported bench checks were not independently reproduced; current-head CI remains pending. State, head, and reviews rechecked before submission.
(Written by OpenAI)
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Merge summary for a1e423a:
(Written by Claude Opus 5.5) |
Problem
moq_net::track::Info::default()(and socreate_track(name, None)andaccept(None)) silently gave a track a millisecond timescale, while@moq/nettreats an omitted timescale as untimed (#4968). The publish-timestamp and lite-07 untimed quests need each track to say whether it is timed before publishers stop inventing timestamps.Approach
moq-net: removedimpl Default for Timescale;Info::default()now hastimescale: None. The lite encoder (TRACK_INFO, frame and datagram send times) and the lite datagram/fetch decoders nameTimescale::MILLIfor an untimed track, as does moq-audio's decode quantum floor.Catalog::default_track_info, also used for MSF now), moq-mux timeline, TS SI tables (ts/si.rs, found by the moq-cli import/export tests), moq-boy status, moq-room chat, moq-stats JSON tracks, moq-bench, moq-cdata_track, moq-ffi json/flate tracks, the moq-tokio and moq-json examples, and the moq-mux doc examples. Media tracks already pin their scale (hang::container::track_info, moq-c/moq-ffi raw tracks at microseconds).Nonenow use a millisecond track (Info::timed()inside moq-net, a localtimed()helper elsewhere), preserving what they tested before. New regression testan_undeclared_timescale_is_untimed;broadcast_moq_lite_05_default_timescalebecamebroadcast_moq_lite_05_untimed_track.doc/setup/upgrade.md(Unreleased),doc/lib/js/net.md, rustdoc onInfo,Info::with_timescale,create_track,hang::container::track_info. Quest file deleted and references updated;publish-timestampandlite-untimedare now unblocked.create_track(name, None)now declareTimescale::MILLI. That includes the io_uring session and WebTransport tests, the relay io_uring runtime tests, and the benches and loom tests that write timestamps. Tracks that never stamp a frame stay untimed. Mergedorigin/mainagain (through quest(m1): plan #4349 follow-ups after the reporter's re-run #5325): main's new tests that stamp frames on aNonetrack (moq-net auth/restart/route_change/finite_relay/late_lower_group/empty_group, the lite and ietf serve tests, moq-bench drain, moq-boy input, moq-relay auth_lifetime, moq-hls routes, the moq-mux TS SI and fMP4 clock-join tests) now declare milliseconds too. moq-ffi's JSON/flate entry points moved to theadoptconstructors on main; their tracks come from the binding'screate_track, which pins microseconds.Decision
CI failed because
lite_session_over_the_workerwritesTimestamp::ZEROon a track whose timescale is now untimed.TimestampMismatchrefusal. These are transport and bench paths, not untimed-track tests, so the track gets an explicit millisecond timescale, whichNoneused to mean.Left for the maintainer, unchanged:
with_timescalecall; no new public API.rs/moq-mux/AGENTS.mdline. Agent files stay untouched unless a maintainer asks.Impact
moq_net::Timescaleno longer implementsDefault.track::Info::default(),broadcast::Producer::create_track(name, None), andtrack::Request::accept(None)create an untimed track; writing a timestamp to it fails withTimestampMismatch. Declare one withInfo::default().with_timescale(Timescale::MILLI).data_trackstays millisecond-timed; moq-ffi tracks, including those adopted by its JSON and flate producers, come from itscreate_track, which already pins microseconds.Alternatives
track::Info::timed(scale)or aTimescale-to-Infoconversion, so timed publishers don't writeInfo::default().with_timescale(...). Not added: no consumer needed new API to land this, and it can come with the publish-timestamp quest if wanted.Timestamp::ZEROas a placeholder to write untimed frames instead. Rejected: many depend on timestamps (expiry, retention), and a timed track keeps each test's original meaning.Follow-ups
rs/moq-mux/AGENTS.mdstill saysInfo::default()is milliseconds; left alone since agent files need maintainer prompting.Tested
cargo nextest run --locked -p moq-uring --no-fail-fast --test session --test web -j 1: 13 passed, includinglite_session_over_the_worker.cargo nextest run --locked -p moq-relay --features io-uring --no-fail-fast --test runtime_uring -j 1: 6 passed.quest check: 418 documents ok, after mergingorigin/main.origin/mainmerge:cargo nextest run --workspace6008 + 459 + 169 + 97 passed;cargo nextest run -p moq-net -p moq-ffi -p moq-cli2243 passed.just check: passed on the timestamp fixes (5615 + 458 + 157 + 97 Rust tests, JS, Python). The laterorigin/mainmerge did not touch those writers.(Written by Grok 4.7 and Claude Opus 5.5)