Skip to content

quest: plan customer timestamp requests; timestamps optional end to end - #4676

Merged
kixelated merged 3 commits into
mainfrom
claude/customer-feedback-timestamps-e98b3f
Oct 1, 2026
Merged

kixelated merged 3 commits into
mainfrom
claude/customer-feedback-timestamps-e98b3f

Conversation

@kixelated

@kixelated kixelated commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

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-timestamp is renamed to Publishing never invents a timestamp. moq-mux data producers take a broadcast-clock Timestamp (from a clock::Lane, or a consumer's Timed.at) without an ahead-of-now check. Clock::capture(Instant) goes public for callers that hold a capture Instant.
  • js-publish-timestamp follows suit, and @moq/net exports Timed<T>.
  • New quest/m1/js-data-consumer-timestamps.md [S]: the @moq/json and @moq/binary consumers yield Timed<T>.
  • plan-untimed-objects records the decision (faithful absence, publish and subscribe). Its remaining job is to map the fallout into implementation quests.
  • data-consumer-timestamps: at is None for an untimed frame.
  • watch-data-sync moves from m3 to m2, since the customer it was waiting for has arrived. Its data-track-clock blocker finished in fix(mux): data tracks stamp on the catalog's current clock #4668 on dev.
  • The third request, impl Stream for Box<S> in moq_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 Lane on an Anchor built from the fixed catalog clock yields timestamps on the broadcast timeline.

Decisions

Goal: plan the three asks separately?

  • ✅ Yes, split 3 ways
  • Drop one

What do the moq-mux json/binary producers take?

  • ✅ Timed<_, Timestamp> on the broadcast clock, like container::Frame; Clock::capture(Instant) public, keeping the ahead-of-now refusal
  • Generic over Instant or Timestamp
  • Typed clock::Time newtype

Which JS work?

  • ✅ New m1 quest: JS consumer timestamps
  • ✅ Promote watch-data-sync to m2
  • Promote watch-data-sync to m1

impl Stream for Box<S>

  • ✅ Do it in a separate PR
  • XS quest in m1

moq-ffi ahead-of-now check

  • ✅ No check, same as Rust
  • Keep the refusal in FFI

JS consumer return shape

  • ✅ Timed<T> exported from @moq/net, shared with the producers
  • Local object type per package

JS consumer blockers / is at required?

  • Required; wait on untimed
  • Required; don't wait
  • Producers only, consumers follow untimed
  • ✅ (answered) Option<Timestamp> for both publishing and subscribing, surviving the wire, never defaulted to wall clock

Confirm the reversal: at: Option end to end

  • ✅ Yes, Option end to end (media included)
  • Option for data, required for media

lite-05/06 can't encode absence

  • ✅ The encoder writes its send time, documented as a downgrade
  • Refuse the frame

How to turn it into work

  • ✅ Record it in plan-untimed-objects, which still writes the implementation quests
  • Plan implementation now

The 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

  • watch-data-sync reads raw frames and decodes them itself
  • Snapshot consumers stop coalescing
  • An opt-in non-coalescing read
  • ✅ (answered) Two methods. next() yields every state in order and latest() skips to the newest (today's behavior, kept as the optimization), in Rust, JS, and moq-ffi. A reader behind the Lagged budget still jumps to the newest group.

Two methods: which names?

  • ✅ next() in order + latest() skips ahead
  • next() skips ahead + every() in order

Review: 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

… to end

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You'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.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 6803cfdc-4d37-47e1-ac02-ea9ab55c6289

📥 Commits

Reviewing files that changed from the base of the PR and between 732101e and 48aaa97.

📒 Files selected for processing (10)
  • quest/m1/README.md
  • quest/m1/data-consumer-timestamps.md
  • quest/m1/js-data-consumer-timestamps.md
  • quest/m1/js-publish-timestamp.md
  • quest/m1/plan-untimed-objects.md
  • quest/m1/publish-timestamp.md
  • quest/m2/README.md
  • quest/m2/watch-data-sync.md
  • quest/m3/README.md
  • quest/m3/watch-data-sync.md
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@kixelated

Copy link
Copy Markdown
Collaborator Author

Review (head 00a73adaad485a21c10acd50d4a33d02c589b2b1)

Clear planning pass: the 2026-09-30 “timestamp required” reversal is recorded consistently across the publish/consumer/untimed quests, watch-data-sync moves to m2 with the OneTooMany trigger, and the new JS consumer quest matches the Rust side. Spot-checks against current code match the “today” claims (Clock::capture is pub(crate), Clock::stamp fills None with now, JS producers default Timestamp.now(), lite/ietf subscribers fill arrival time, #4668 merged to dev and removed data-track-clock.md there).

Blocking

  1. publish-timestamp.md is aimed at dev but still describes the pre-#4668 stamp path. On dev, mux data producers call Listing::stamp(Timed<_, Instant>) → Clock::stamp (rs/moq-mux/src/catalog/data.rs, rs/moq-mux/src/json.rs). The quest says delete Clock::stamp and adjust Listing::record, but never says to remove/replace Listing::stamp or change producers off Instant. Without that, an implementer following the quest on the stated target branch misses the real entry point #4668 just added.

  2. “Zero delay for a timestamp ahead of now” is underspecified against the estimator. Accepting a slightly-fast source clock is the right product call, but today Estimator::flush feeds lateness into the shared catalog baseline (estimate.rs Baseline::observe). A future PTS makes lateness negative and can inflate sibling tracks’ delay, not merely clamp this track to zero. Say explicitly that ahead-of-now timed writes skip flush (same as untimed), or describe another baseline-safe rule—not only “record zero delay.”

Non-blocking

  • publish-timestamp.md “same contract as container::Frame” is easy to misread: hang::container::Frame.timestamp is a required Timestamp, while producers take Timed<_, Timestamp> with at: Option. Same clock type, not same optionality; media Option is plan-untimed’s job—worth one clarifying phrase.
  • data-consumer-timestamps.md still opens with “Every moq_net::Frame has a timestamp” while this PR decides Option end to end (media included). Mark that sentence as today’s wire/model, or point at plan-untimed for Frame/Info becoming optional.
  • Required edges still point at the planner quest (plan-untimed-objects) rather than the impl quests it has yet to write. Fine as a temporary state if the planner’s next edit rewires those lists when it splits—just don’t leave publish/consumer blocked on a quest that deletes itself without a handoff.

Cross-PR

  • #4668 on dev: watch’s data-track-clock blocker claim is accurate.
  • #4669 (one catalog timeline): related to Lane/Anchor broadcast timestamps for KLV/MAVLink; doesn’t block this quest PR.

CI: Check/Test still pending at review time (docs-only; unlikely to matter).

Verdict: ITERATE (reviewed 00a73adaad485a21c10acd50d4a33d02c589b2b1)

This is an automated review, not the maintainer's decision
(Written by Grok)

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. [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.

  2. [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>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Addressed both findings in 07f4647:

  1. Snapshot consumers stop coalescing, in Rust and JS. They yield every state in order with its timestamp, and the caller keeps the latest. A reader behind the Lagged budget still jumps to the newest group. This is recorded in data-consumer-timestamps and js-data-consumer-timestamps; the JS one is resized to [M].
  2. An untimed record adds no timestamp wait of its own, but still waits for the records before it. Both sequences are now test cases in watch-data-sync.

(Written by Claude Opus 5.5)

…ping ahead

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Follow-up in 48aaa97: instead of dropping the skip-ahead, snapshot consumers get two reads. next() yields every state in order; latest() skips to the newest, today's behavior, kept as the optimization. The names are the same in Rust, JS, and moq-ffi.

(Written by Claude Opus 5.5)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Re-review (head 07f46471bb618dcaf9a3aac6211bbc43bcbed668)

Notable push since 00a73ada: snapshots stop coalescing (Rust + JS consumer quests, S→M on the JS one), and watch-data-sync spells out untimed stream order (timed@11s then untimed both release at an 11s playhead). Spot-check: today’s moq-json / moq-flate / @moq/json snapshot consumers do coalesce to the latest (and drain to the newest group), so the “today” claim matches code.

Prior findings

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)

  1. 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 / JS setGroups({ start: latest }) do on every poll today—not only after Lagged—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 its at); jump to the newest group only on Lagged / 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.md still 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 frame at. Drop or rewrite that sentence so it doesn’t fight the new decision.
  • Untimed-first / untimed-only stream sequences in watch-data-sync are 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 / #4669 notes 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)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Review (head 48aaa97e4e186de3859524df97c42df29ac93be7)

Push delta (from 00a73ada): snapshot consumers get next() (in-order) + latest() (today's coalesce), JS quest resized [S]→[M], and watch-data-sync records untimed stream/snapshot ordering with test cases. That API split matches the playhead need (a 9s state must stay reachable when 11s is already buffered); spot-check against dev confirms today's coalesce (moq_json::snapshot::Consumer::poll_next, js/json drain loop, moq_flate::snapshot jump-to-newest).

The commit note on 07f46471 ("Addressed both findings") answers the PR's own snapshot/untimed decisions, not the prior Grok blockers below — those files were not touched in this push.

Blocking

  1. publish-timestamp.md still omits the post-#4668 Listing::stamp path (unchanged since 00a73ada). On dev, mux producers take Timed<_, Instant> and go through Listing::stamp → Clock::stamp (rs/moq-mux/src/{json,binary}.rs, catalog/data.rs). The quest still only says delete Clock::stamp and adjust Listing::record. An implementer on the stated target branch will miss the real entry point. Spell out: remove/replace Listing::stamp, switch producers to Timed<_, Timestamp>, and drop the Instant→stamp hop.

  2. "Zero delay for a timestamp ahead of now" is still underspecified vs the shared estimator (unchanged). Estimator::flush → Baseline::observe feeds lateness into the catalog-wide minimum (estimate.rs). A future PTS makes lateness negative and can inflate sibling tracks' delay, not merely clamp this track. Say ahead-of-now timed writes skip flush (same as untimed), or give another baseline-safe rule — not only "record zero delay."

Non-blocking

  • JS async iterator vs latest(): js-data-consumer-timestamps.md says "next() and the async iterator yield Timed<T>" and separately that snapshots get next()/latest(). State explicitly that for await follows in-order next(), not latest(), so today's drain-to-head iterator callers know they must switch.
  • watch-data-sync "picked from the in-order states": with next() yielding every state, the reader must buffer/skip to the newest ≤ playhead (not release every intermediate at wall time, and not call latest() which ignores the playhead). One sentence on that selection rule would lock the intended algorithm.
  • Prior NBs still open: "same contract as container::Frame" optionality mismatch; opening line "Every moq_net::Frame has a timestamp" vs Option end-to-end; Required edges still on planner plan-untimed-objects.

Cross-PR

  • #4668 on dev: Listing::stamp / data-track-clock claims remain accurate; blocker (1) is exactly the quest gap that merge created.
  • #4669 / #4677: related catalog/timeline and Box<S>: Stream; neither unblocks the two items above.

CI: Check/Test queued at review time (docs-only).

Verdict: ITERATE (reviewed 48aaa97e4e186de3859524df97c42df29ac93be7)

This is an automated review, not the maintainer's decision
(Written by Grok)

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 coalescing latest(). 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)

@kixelated

Copy link
Copy Markdown
Collaborator Author

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 Timed<T>. Snapshot consumers get an in-order next() beside latest(). watch-data-sync moves to m2. plan-untimed-objects records faithful absence (lite-05/06 write send time). impl Stream for Box<S> is #4677. Every decision is listed in the PR description.

(Written by Claude Opus 5.5)

@kixelated
kixelated enabled auto-merge (squash) October 1, 2026 22:22
@kixelated
kixelated merged commit cbaa563 into main Oct 1, 2026
3 checks passed
@kixelated
kixelated deleted the claude/customer-feedback-timestamps-e98b3f branch October 1, 2026 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant