Repository navigation
quest: plan customer timestamp requests; timestamps optional end to end - #4676
Conversation
… to end Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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 5 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (10)
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 |
Review (head
|
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 00a73ad
Direction: preserving timestamp absence and sharing Timed<T> are sensible. Two playback contracts need adjustment before implementation:
-
[P2] Preserve snapshot states until the playhead can select them. quest/m2/watch-data-sync.md:27 now builds playback on consumers whose planned result is the last decoded frame. The existing JSON consumer drains buffered frames and returns only the latest reconstruction; binary skips to the newest group. With 9s and 11s states buffered at a 10s playhead,
next()exposes only 11s and loses the required 9s state. Adding its timestamp cannot implement “newest value at or before the playhead.” Explicitly plan a non-coalescing read path, or let watch retain raw frames and use the existing decoder to materialize eligible states. -
[P2] Resolve mixed timed/untimed stream ordering. quest/m2/watch-data-sync.md:17–19 promises both in-order stream delivery and untimed release on arrival. If record A is timed at 11s and the following B is untimed while the playhead is 10s, immediate B delivery overtakes A; queuing B violates the arrival rule. Define untimed stream records as having no additional timestamp wait while still waiting for preceding records, or explicitly disallow mixed timing. Include this sequence in the planned acceptance cases.
Verification: static review of all nine changed quest files and relevant current code; the JSON snapshot behavior also matches current dev. This PR changes plans only. No builds, runtime tests, or interoperability tests were run.
(Written by OpenAI)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed both findings in 07f4647:
(Written by Claude Opus 5.5) |
…ping ahead Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Follow-up in 48aaa97: instead of dropping the skip-ahead, snapshot consumers get two reads. (Written by Claude Opus 5.5) |
Re-review (head
|
| Finding | Status |
|---|---|
Listing::stamp / Instant producers omitted after #4668 |
Still open — publish-timestamp.md untouched |
Ahead-of-now “zero delay” vs shared Estimator baseline |
Still open — same |
container::Frame optionality wording |
Still open (NB) |
“Every Frame has a timestamp” vs end-to-end Option |
Still open (NB) |
| Required edges still point at the planner quest | Still open (NB) |
Blocking (new)
- Stop-coalescing does not say to stop the always-drain-to-newest-group path. The quest’s own failure mode (“a 9s state is lost when 11s is already buffered”) is what
poll_next_group/ JSsetGroups({ start: latest })do on every poll today—not only afterLagged—in both moq-json and moq-flate (rs/moq-json/src/snapshot/consumer.rs,rs/moq-flate/src/snapshot/consumer.rs,js/json/src/snapshot/consumer.ts). The new text only carves out “falls behind the drift budget (Lagged) still jumps.” An implementer can keep draining groups and only yield every frame inside the newest one, and the 9s/11s cross-group case stays broken. Say explicitly: walk groups in order (yield every reconstructed state with itsat); jump to the newest group only onLagged/Old/Evicted(or whatever gap set you mean)—not on every poll when a newer group is already buffered.
Non-blocking (new)
data-consumer-timestamps.mdstill says “A snapshot consumer returns the timestamp of the frame it decoded last.” That matched coalesce-to-latest; after this push each yielded state should carry that state’s frameat. Drop or rewrite that sentence so it doesn’t fight the new decision.- Untimed-first / untimed-only stream sequences in
watch-data-syncare only implied (“adds no timestamp wait of its own”). The timed→untimed test note is good; add the untimed-leading and untimed-only cases so release-on-arrival isn’t left to guesswork. - Late-joiner cost: yielding every buffered state (deserialize + emit) is a real catch-up tax the current consumers intentionally avoided. Fine if watch (or the caller) is the coalescer, but worth one line that live “current value only” callers keep the last yield—and that this is an accepted cost of playhead sync.
Cross-PR / CI
- Prior
#4668/#4669notes unchanged; this push doesn’t touch them. - Check/Test were still in progress at review time (docs-only).
Verdict: ITERATE (reviewed 07f46471bb618dcaf9a3aac6211bbc43bcbed668)
This is an automated review, not the maintainer's decision
(Written by Grok)
Review (head
|
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 48aaa97
Incremental review since 00a73ad.
Both findings from my previous review are addressed in the plan:
- Snapshot history: Rust/bindings, lines 28–40 and JS, lines 18–22 now separate in-order
next()from coalescinglatest(). This preserves eligible buffered states for playhead selection while keeping the current-value optimization. - Mixed timing: watch-data-sync.md:20–24 explicitly queues untimed records behind predecessors and calls for coverage of both stream and snapshot sequences.
Direction: the two-method split and ordered untimed delivery resolve the earlier playback-contract conflicts. No new actionable bugs found in the four-file incremental change.
Verification: static comparison of both new commits, the updated plans, and nearby Rust/JS snapshot consumers, JS stream consumers, and watch Sync code. This changes plans only; no builds, runtime tests, or interoperability tests were run.
(Written by OpenAI)
|
Summary before merge: this plans the three OneTooMany requests and makes timestamps optional end to end. Moq-mux data producers take broadcast-clock timestamps. The JS consumers return (Written by Claude Opus 5.5) |
Plans three customer requests from OneTooMany. Planning them also reversed the 2026-09-30 decision that publishing requires a timestamp: it's now optional end to end.
publish-timestampis renamed to Publishing never invents a timestamp. moq-mux data producers take a broadcast-clockTimestamp(from aclock::Lane, or a consumer'sTimed.at) without an ahead-of-now check.Clock::capture(Instant)goes public for callers that hold a captureInstant.js-publish-timestampfollows suit, and@moq/netexportsTimed<T>.quest/m1/js-data-consumer-timestamps.md[S]: the@moq/jsonand@moq/binaryconsumers yieldTimed<T>.plan-untimed-objectsrecords the decision (faithful absence, publish and subscribe). Its remaining job is to map the fallout into implementation quests.data-consumer-timestamps:atisNonefor an untimed frame.watch-data-syncmoves from m3 to m2, since the customer it was waiting for has arrived. Itsdata-track-clockblocker finished in fix(mux): data tracks stamp on the catalog's current clock #4668 ondev.impl Stream for Box<S>inmoq_mux::catalog, is too small for a quest and gets its own PR.Related: #4669 (one timeline for every source on a catalog). It fits item 1, because a
Laneon anAnchorbuilt from the fixed catalog clock yields timestamps on the broadcast timeline.Decisions
Goal: plan the three asks separately?
What do the moq-mux json/binary producers take?
Timed<_, Timestamp>on the broadcast clock, likecontainer::Frame;Clock::capture(Instant)public, keeping the ahead-of-now refusalInstantorTimestampclock::TimenewtypeWhich JS work?
watch-data-syncto m2watch-data-syncto m1impl Stream for Box<S>moq-ffi ahead-of-now check
JS consumer return shape
Timed<T>exported from@moq/net, shared with the producersJS consumer blockers / is
atrequired?Option<Timestamp>for both publishing and subscribing, surviving the wire, never defaulted to wall clockConfirm the reversal:
at: Optionend to endlite-05/06 can't encode absence
How to turn it into work
plan-untimed-objects, which still writes the implementation questsThe MAVLink case (a carried-over timestamp lines up only on a broadcast that shares the source's clock): ✅ documented as a constraint, and the customer will be asked where their derived track lives.
Review: snapshot consumers coalesce, so a playhead can't select an older buffered state
next()yields every state in order andlatest()skips to the newest (today's behavior, kept as the optimization), in Rust, JS, and moq-ffi. A reader behind theLaggedbudget still jumps to the newest group.Two methods: which names?
next()in order +latest()skips aheadnext()skips ahead +every()in orderReview: mixed timed and untimed stream records. An untimed record adds no timestamp wait of its own, but still waits for the records before it.
Impact
This PR changes quests only. Public API: none. Wire: none. The planned work breaks the publish and consume APIs on
dev, and adds absent-timestamp encoding in lite-07.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code