feat(json): let a snapshot publisher cut a group - #3146
Conversation
![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: 9e39e69d90
ℹ️ 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".
| /// The log continues: no replacement group opens until the next [`append`](Self::append), which | ||
| /// starts a cold DEFLATE window that a consumer joining there can decode on its own. That is what | ||
| /// bounds the log's history, since a consumer reads whole groups. |
There was a problem hiding this comment.
When users follow this new cut API, the public documentation becomes internally contradictory: rs/moq-json/src/stream/producer.rs:13 still says every record is published "in a single group," while rs/moq-json/src/stream/consumer.rs:13-15 says this Producer writes the whole log into one group and attributes rolling only to lower-level Encoder users. Update those public type docs to describe the new producer-managed cuts and max_age behavior so consumers do not build around the obsolete single-group invariant.
AGENTS.md reference: AGENTS.md:L107-L109
Useful? React with 👍 / 👎.
9e39e69 to
7adfdc8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7adfdc8e52
ℹ️ 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".
| match moq_net::Timestamp::now().checked_sub(opened) { | ||
| Ok(age) => age.as_micros() >= max_age.as_micros(), |
There was a problem hiding this comment.
On a native host whose wall clock is before 2020 when the process starts, Timestamp::now() saturates to zero in rs/moq-net/src/model/time.rs, so this subtraction continues reporting an age of zero until the monotonic elapsed time catches up to 2020. In that environment, max_age never rolls groups in either JSON publishing mode, potentially for years; store a monotonic Instant for elapsed-age checks while retaining the timestamp separately for frame presentation. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
7adfdc8 to
9b78821
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b78821bcd
ℹ️ 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".
| // The third `maxAge` bounding a group's age, and the only one that decides where a group *ends*: | ||
| // a track's `maxAge` is how long a publisher keeps a closed group, and a subscription's is how | ||
| // long a subscriber waits for one. | ||
| maxAge?: Time.Milli; |
There was a problem hiding this comment.
The newly exported Config.maxAge member is preceded only by // comments, so its detailed behavior will not appear as API documentation for consumers of @moq/json. Convert the block to a /** ... */ doc comment, as required for exported TypeScript members. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L107-L107
Useful? React with 👍 / 👎.
A `moq-json` snapshot publisher had no way to force a group boundary. The encoder rolls on its own delta budget and frame cap, so a caller that knows the deltas already written should stop being provisional -- because it is about to store them, or because a joiner should not have to replay them -- had no way to say so. Add `snapshot::Producer::cut()`. It FINs the open group and opens no replacement: the next `update` does, emitting a full snapshot as frame 0 even when the value is unchanged, so a consumer joining there reads the whole value and none of the deltas that preceded the cut. Cutting with no group open is a no-op, so a caller can cut on its own schedule without tracking what it has published, and it is inert with deltas disabled, where every frame already gets its own group. Deliberately not included: an age-based bound, and the same method on `stream`. A cut cannot express a sliding window on an append-only log -- re-seeding a new group with the retained records is indistinguishable from new data, so a live consumer receives them twice -- and that is what the timeline actually needs. It gets a windowed log of its own instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
9b78821 to
fd6c14b
Compare
kixelated
changed the title
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. |
1 participant
Summary
A
moq-jsonsnapshot publisher had no way to force a group boundary. The encoder rolls on its own delta budget and frame cap, so a caller that knows the deltas already written should stop being provisional — because it is about to store them, or because a joiner should not have to replay them — had no way to say so.snapshot::Producer::cut()FINs the open group and opens no replacement. The nextupdatedoes, emitting a full snapshot as frame 0 even when the value is unchanged, so a consumer joining there reads the whole value and none of the deltas that preceded the cut. Cutting with no group open is a no-op, so a caller can cut on its own schedule without tracking what it has published, and it is inert withdeltaRatioat 0, where every frame already gets its own group.Deliberately not included
This PR originally also carried an age-based bound (
max_age) and the same method onstream. Both are dropped.A cut cannot express a sliding window on an append-only log. Re-seeding a new group with the retained records is indistinguishable from new data on the wire — a
streamconsumer yields every frame in order across group boundaries — so a live consumer receives them twice. In this repo that is worse than a duplicate:moq-hlsreads a non-increasingsegmentas "the publisher restarted" and clears its whole playlist window (export/segments.rs:137,export/renditions.rs:185).Since the broadcast timeline is the only
streamuser in either language, a cut there would have shipped with no consumer and a real footgun. The timeline gets a windowed log with tagged reset/push/pop ops instead, tracked separately.Public API changes
Additive; nothing renamed, removed, or signature-changed.
moq_json::snapshot::Producer::cut()(new)@moq/jsonSnapshot.Producer.cut()(new)Wire behavior changes
No encoding change, and no change at all unless a caller opts in by calling
cut(). When one does, a peer sees a group boundary it would not otherwise see, and the next frame is a full snapshot rather than a merge patch. Both are shapes the format already produces whenever the encoder's own budget rolls a group, so nothing new has to be understood by a consumer. No draft update needed.Test plan
just checkandjust test(1933 tests, 1 skipped) green.(written by claude-opus-5)