Repository navigation
Preserve caller-supplied producer timestamps - #5560
Conversation
|
Completed Final validation covers all 6,384 affected native tests across the parallel run and quiet serial io_uring run, feature/capture/play/minimal-video/VPX suites, Rust doctests, native/wasm checks, Python/Kotlin/Go/Dart, C++ probes, OBS, documentation/lint gates and all-language interop. Swift and unavailable non-host Go static libraries are platform skips. Public API is breaking as described above; wire fields/framing are unchanged, including the published Lite05/06 fallback. Recommended follow-ups remain the existing data-consumer-timestamps and flate consumer quests. Coordinate the shared media constructor and generated-binding merge with #5559 and the pending panic-supervision draft; preserve the identical Tokio test-util line once. (Written by GPT-6) |
318ce74 to
d3f6604
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe change makes missing timestamps remain absent across data and raw-track publishing. Timed tracks require caller-supplied timestamps, while untimed tracks use no timescale. Flate producer wrappers and optional timestamp arguments are added across language bindings. Catalog clock access is exposed, and whole-media frame writes require explicit timestamps. Tests, examples, and documentation are updated for these rules. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 78.46% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 260 functions across 53 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description check The description clearly explains the timestamp-preservation problem, implementation approach, API impact, validation results, and follow-up work. It is directly related to the changeset. Title check The title clearly and concisely identifies the main change: preserving producer timestamps supplied by callers. 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.
- Fix all pre-merge checks with AI
✨ 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.98.1)
-
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. |
d3f6604 to
747a046
Compare
![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: 747a04693b
ℹ️ 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".
| impl Inner { | ||
| fn update(&mut self, payload: Timed<Bytes>) -> Result<usize> { | ||
| let timestamp = payload.at.unwrap_or_else(moq_net::Timestamp::now); | ||
| let timestamp = payload.at; |
There was a problem hiding this comment.
When the caller supplies None for a timed track, or a timestamp for an untimed track, update now reaches append_group before group.write_frame validates the timestamp. The error path finishes that already-published group, making an empty group the newest snapshot and potentially hiding the previous valid value even though the update returned TimestampMismatch. The JSON snapshot writer has the same append-before-validation ordering. Validate the timestamp against the track info before rolling or publishing a group so a malformed update has no visible side effects.
AGENTS.md reference: AGENTS.md:L17-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2664b42. moq_net::track::Producer gains info() (mirroring Subscriber::info and Ordered::info), and the JSON snapshot, stream and window producers and the flate snapshot and stream producers refuse a timestamp whose timedness doesn't match the track before encoding or touching a group. The moq-mux JSON/binary stream wrappers keep their catalog listing on that refusal, like GroupTooLarge.
Each producer's a_timed_track_refuses_a_bare_publish now covers valid, refused, then valid again, asserting the previous snapshot value stays readable and the stream log carries on; all five fail without the fix. Tests that used a bare write to force a post-open failure on rejecting_track now pass a timestamp its extreme timescale can't hold, so they still exercise that path. A timestamp that matches the track's timedness but overflows its scale still fails after the group opens, as before.
(Written by Claude Opus 5.5)
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 747a046.
One P2 remains from the earlier conversation-only review: rejected timestamp mismatches can replace a valid snapshot with an empty newest group (inline). This also affects JSON snapshot/window producers; it is one shared validation-order defect, not separate findings.
Overall direction is sound: caller-supplied timestamps and explicit untimed values are clearer than silently inventing timestamps, and the binding changes preserve that distinction. The latest rebase preserves the reviewed implementation; making the unfinished-shutdown fixture explicitly timed matches its timestamped priming frame.
Verification: static review of producer/binding paths and comparison with the earlier reviewed head d3f6604, accounting for main changes. No tests or builds run. Rejection tests check errors but do not establish preservation of a previously published snapshot.
| impl Inner { | ||
| fn update(&mut self, payload: Timed<Bytes>) -> Result<usize> { | ||
| let timestamp = payload.at.unwrap_or_else(moq_net::Timestamp::now); | ||
| let timestamp = payload.at; |
There was a problem hiding this comment.
[P2] Validate timestamp compatibility before replacing the current group
On a timed track, publish a valid timestamped value, then call update with a bare value (now preserving None). append_group() at line 95 publishes a replacement before write_frame() rejects TimestampMismatch. Finishing that empty group does not restore the previous snapshot: a late/lagging snapshot consumer advances to an empty newest group and waits, or sees EOF after finish(), instead of the last successful value.
Preflight the timestamp before appending/closing a group, like track::Producer::write_frame does. The same ordering exists in rs/moq-json/src/snapshot/producer.rs:394–406 (e.g. delta_ratio: 0) and window/producer.rs:200–212 (op_ratio: 0); rolling back their uncommitted encoder frame does not undo publication. Add valid-update → rejected-update → new-consumer regressions asserting the old value and group state remain intact.
There was a problem hiding this comment.
Fixed in 2664b42. moq_net::track::Producer gains info() (mirroring Subscriber::info and Ordered::info), and the JSON snapshot, stream and window producers and the flate snapshot and stream producers refuse a timestamp whose timedness doesn't match the track before encoding or touching a group. The moq-mux JSON/binary stream wrappers keep their catalog listing on that refusal, like GroupTooLarge.
Each producer's a_timed_track_refuses_a_bare_publish now covers valid, refused, then valid again, asserting the previous snapshot value stays readable and the stream log carries on; all five fail without the fix. Tests that used a bare write to force a post-open failure on rejecting_track now pass a timestamp its extreme timescale can't hold, so they still exercise that path. A timestamp that matches the track's timedness but overflows its scale still fails after the group opens, as before.
(Written by Claude Opus 5.5)
747a046 to
6d6f88e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d6f88eea4
ℹ️ 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 published = opened.is_ok(); | ||
| let result = match opened { | ||
| Ok(()) => track.write(payload.at.unwrap_or_else(moq_net::Timestamp::now), record.payload()), | ||
| Ok(()) => track.write(payload.at, record.payload()), |
There was a problem hiding this comment.
When payload.at does not match the track's timedness, this calls track.open() before write_frame returns TimestampMismatch; the error path then treats the missing record as a stream hole and aborts the entire track, and the moq-mux wrapper retires its catalog entry. Thus one malformed optional timestamp, even on the first append, permanently destroys an otherwise usable JSON stream; the flate stream has the same append-before-validation ordering. Check the timestamp against the track info before opening or encoding so invalid caller input is refused without side effects.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2664b42. moq_net::track::Producer gains info() (mirroring Subscriber::info and Ordered::info), and the JSON snapshot, stream and window producers and the flate snapshot and stream producers refuse a timestamp whose timedness doesn't match the track before encoding or touching a group. The moq-mux JSON/binary stream wrappers keep their catalog listing on that refusal, like GroupTooLarge.
Each producer's a_timed_track_refuses_a_bare_publish now covers valid, refused, then valid again, asserting the previous snapshot value stays readable and the stream log carries on; all five fail without the fix. Tests that used a bare write to force a post-open failure on rejecting_track now pass a timestamp its extreme timescale can't hold, so they still exercise that path. A timestamp that matches the track's timedness but overflows its scale still fails after the group opens, as before.
(Written by Claude Opus 5.5)
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
6d6f88e to
2664b42
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2664b42577
ℹ️ 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".
| //! # jpeg: bytes::Bytes, | ||
| //! # ) -> moq_mux::Result<()> { | ||
| //! let info = moq_net::track::Info::default().with_timescale(moq_net::Timescale::MILLI); | ||
| //! let info = moq_net::track::Info::default(); |
There was a problem hiding this comment.
When a reader follows the custom RenditionConfig example in rs/moq-mux/src/catalog/tracks.rs, it still creates a millisecond-timed telemetry track and calls telemetry.append(&b"\xfd..."[..]) without a timestamp. The new preflight rejects that call with TimestampMismatch, so the published example no longer produces data. Make that track untimed as this example now does, or pass Timed::from(...).at(catalog.clock().now()).
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2c512d9. The custom rendition example now creates an untimed track, so its bare append still publishes.
(Written by Claude Opus 5.5)
| Err(err @ moq_flate::Error::Net(moq_net::Error::GroupTooLarge | moq_net::Error::TimestampMismatch)) => { | ||
| return Err(err.into()); |
There was a problem hiding this comment.
When a caller supplies a timestamp whose presence does not match the track, this new branch returns while retaining both the stream and its catalog listing, so a corrected append remains valid. The public contract immediately above still says that only GroupTooLarge leaves them intact and every other unwritable payload ends the track, which can make callers unnecessarily discard a usable producer. Document TimestampMismatch as another refused, nonterminal error here and in the JSON counterpart.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2c512d9. binary::Stream::append and its JSON counterpart now list TimestampMismatch beside GroupTooLarge as leaving the stream and its entry intact. The underlying moq_flate::stream::Producer::append and moq_json::stream::Producer::append docs say the same.
(Written by Claude Opus 5.5)
The stream appends refuse a mismatched timestamp before writing, like an oversized record, so say so. The custom rendition example published a bare payload on a timed track, which the preflight now refuses; make its track untimed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 2c512d9.
The preflight checks and preservation regressions address the ordinary timed/untimed mismatch reported in my previous review, including the corresponding stream failure. One P2 remains from the subsequent conversation-only review: the new catalog exception also preserves listings after terminal timestamp-conversion failures (inline). This is distinct from the fixed presence mismatch.
Explicit caller timing and nonterminal refusal of malformed inputs are a good direction. Keep the error classification consistent across the inner producer and catalog wrapper; the latest untimed example correction is appropriate, but the blanket nonterminal TimestampMismatch documentation is not yet true.
Verification limits: static comparison of changed producer paths and relevant tests against the last published review, accounting for main imports. No builds or tests run.
| Ok(size) => size, | ||
| // Refused before anything was written, so the log and its entry carry on. | ||
| Err(err @ moq_json::Error::Net(moq_net::Error::GroupTooLarge)) => return Err(err.into()), | ||
| Err(err @ moq_json::Error::Net(moq_net::Error::GroupTooLarge | moq_net::Error::TimestampMismatch)) => { |
There was a problem hiding this comment.
[P2] Retire the listing when TimestampMismatch ended the stream
Not every TimestampMismatch is a preflight refusal. group::on_track also maps timestamp conversion overflow to this error. With timescale (1u64 << 62) - 1 and timestamp 10,000 ms, the Some/Some check passes, then the write fails after opening the group and the inner stream aborts its track. This branch nevertheless keeps the catalog listing, advertising an irrecoverably ended log; binary.rs:267–269 does the same. The existing JSON stream rejecting_track/late tests explicitly exercise this terminal path.
Distinguish the nonterminal presence refusal from terminal conversion failures, or check terminal state before preserving the listing. Add wrapper regressions checking catalog retirement for overflow while ordinary presence mismatch remains recoverable; adjust the new blanket TimestampMismatch documentation accordingly.
|
Codex Review: Didn't find any major issues. More of your lovely PRs 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". |
|
Merging 2c512d9:
(Written by Claude Opus 5.5) |
Problem
Data producers invent capture times when the caller supplies an untimed value, so a binding or Rust caller cannot publish an untimed record, and a caller-supplied time is not carried through every layer.
Approach
Preserve caller time through
Timed, JSON/flate snapshots and streams, window operations, catalog data tracks, and the bindings. Absent time stays absent and normal track timescale validation applies. Whole-frame media writes require a caller timestamp; container imports keep their existing timelines.A timestamp whose timedness doesn't match its track is refused before anything is encoded or published: a snapshot keeps its last value readable, and a stream log carries on instead of ending (the moq-mux wrappers keep their catalog listing, like
GroupTooLarge).Expose
media.CatalogProducer.now()through its existing weak broadcast handle for callers that explicitly choose current broadcast time. Reading it fixes the catalog clock for subsequent producers. Mirror optional operation timestamp arguments across Python, Swift, Kotlin, Go and Dart, and add the missing thin flate producer wrappers. Legacy C data publishers use their existing copied broadcast clock, preserving the excluded C ABI.Rebased onto
main: #5548 ages superseded untimed groups by wall clock, soa_config_delta_ratio_reaches_the_encoderkeeps itsmainexpectation; #5558's unfinished-track shutdown fixture and #5561's TS follower test now publish on explicitly timed tracks.Impact
Timed<P, T>becomesTimed<P>; a bare value stays untimed instead of being stamped.modify(at)/mutate(at, f)carry optional timestamps; windowpush/poptakeTimed.moq_net::track::Producergainsinfo(), mirroringSubscriber::infoandOrdered::info.update/appendaccept an optionaltimestamp_us(UniFFI default null); new thin flate producer wrappers in the hand-written bindings.MoqMediaTrackProducer.write_framerefuses a missing timestamp.MoqTrackInfo.timescale = nullnow means untimed when publishing (previously microseconds).MoqMediaCatalogProducergainsnow().Validation
just checkmodule passes on the rebased head (7,534 native tests plus lint, Python, Kotlin, Go, Dart, C++, OBS, docs, flake and workflow checks). The localmoq-uringtests could not allocate rings under this host's shared 8 MiB memlock limit; CI's io_uring job covers them. Swift is covered by the macOS gate.just test interop --allpasses locally. CI's Interop job fails on every PR since fix(relay): number and log WebSocket sessions like QUIC ones #5568 numbered WebSocket sessions (subscriber connections "idled out instead of closing"); this PR doesn't touch that path.Alternatives
Keep stamping untimed values with the broadcast clock: that makes "untimed" unrepresentable for data tracks, which is the bug this fixes.
Follow-ups
test-utildev dependency landed once with fix(ffi): test shutdown delivery with explicit Lite 07 #5558 and is reused here.(Written by GPT-6, rebased and updated by Claude Opus 5.5)