Skip to content

refactor(net)!: make group delivery order a handle, and retire the Ordered wire field - #3099

Merged
kixelated merged 10 commits into
devfrom
claude/net-ordered-handle
Aug 29, 2026
Merged

refactor(net)!: make group delivery order a handle, and retire the Ordered wire field#3099
kixelated merged 10 commits into
devfrom
claude/net-ordered-handle

Conversation

@kixelated

@kixelated kixelated commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Implements the settled design in #3086. Bases on dev; the moq-ffi bindings regeneration and the remaining doc/lib binding pages follow as an immediately stacked PR.

Summary

  • Subscriber::ordered(self) -> track::Ordered. The delivery order is a handle, not a method choice. Consuming self makes interleaving the two cursors unrepresentable rather than merely discouraged. The plain Subscriber keeps the arrival cursor (recv_group) and datagrams; the relay forwarders and moq-mux stay on it.
  • The cursors are decoupled. recv_datagram no longer bumps the sequence cursor: datagrams are unordered by construction, so there is nothing for a sequence cursor to do with them.
  • read_frame is removed from the track level. It was two different functions under one name (plain: one frame per group then skip; spliced: drain every frame in sequence), which is why it is removed rather than moved. Callers loop next_group() + group.read_frame().
  • The ordered cursor bursts instead of discarding. A group already cached costs nothing to deliver, and dropping it puts a hole in the sequence a decoder is reading. Subscription::max_age now bounds only how long a handed-out group may block, which group expiry already enforced (it only convicts a read that is Pending). This is the policy moq-mux's container consumer has always used. The arrival cursor keeps writing a backlog off, which is what lets a relay shed one. The spliced subscriber (route takeovers) honors the same contract: each segment is driven through its own capped sequence cursor with one group staged per segment, and the lowest sequence across segments is delivered next, so a backlog survives a takeover instead of being shed by the arrival path's budget (moq-json's lossless stream rides this).
  • A group is expired only once it provably cannot overlap the budget. Max age judged a group by where it started, convicting it for being behind rather than for having nothing left to give. Priority already transmits newer groups first, so an older group uses only leftover capacity and closes the gap faster than the live edge advances. A group is now measured by its reach — its successor's first frame timestamp, since it cannot present past where the next group begins. Its own frames prove nothing: durations are not on the wire, so a group's last timestamp is where that frame starts. Reach is exclusive, hence >=, which also makes a zero budget fall out of the general rule instead of needing a special case. This retires all three prior measures (the original first-frame one, feat(moq-net): enforce the subscriber latency budget #2890's reader-position preference, and an interim newest-frame one that hung a reader outright). The wall-clock measure is removed outright: expiration is timestamp-only, and wall-clock reclamation of idle content is the retention cache's own policy (Publisher Max Age, the pool's access-time expiry) rather than part of the subscription rule. The reach rule makes the candidate's own timestamp irrelevant, so an empty or stalled group is still bounded by its stamped successor; a chain with no stamped successor expires nothing and is bounded by the cache instead.
  • Ordered carries datagrams too. They are unordered by construction and live on a separate cursor, so the choice of group order says nothing about them; withholding them only forced a track using both channels to subscribe twice.
  • Ordered is removed from the wire and the model (see below).
  • moq-transcode's shared decode feed reads in arrival order. A sequence cursor would burst a retained backlog through the decoder; the arrival cursor's budget keeps dropping whatever falls behind the live edge, matching the old next_group live-edge behavior. Group order is irrelevant there because every group decodes independently from its own keyframe.

Two premises in the issue's design comment were written against main and are stale on dev, so they are not part of this PR: max_age is already enforced (#2890, TrackState::is_stale + live_edge), and the Max Age draft rename already landed.

Public API changes

Breaking, which is what sends this to dev:

Item Change
moq_net::track::Subscriber::next_group / poll_next_group moved to track::Ordered
moq_net::track::Subscriber::read_frame / poll_read_frame removed
moq_net::track::Subscription::ordered, with_ordered removed
moq_net::track::Info::ordered, with_ordered removed
moq_net::lite::Priority::ordered, PriorityQueue::set_ordered removed
moq_net::lite::{Subscribe,SubscribeUpdate,SubscribeOk,TrackInfo}::ordered removed
MoqSubscription.ordered, MoqTrackInfo.ordered (moq-ffi) removed
moq_track_info.ordered, moq_subscription.ordered (libmoq C ABI) removed — a struct layout break for a prebuilt consumer
Track.Subscriber.{nextGroup,readFrame,readFrameSequence,readString,readJson,readBool} (js/net) moved to Track.Ordered
Track.Info.ordered, Track.Subscription.ordered (js/net) removed

Added: moq_net::track::Ordered and Track.Ordered (js/net), each with the full group cursor plus recv_datagram/recvDatagram. (The wire-layer version gates for the retired byte are crate/module-private, not public API.)

moq-ffi's MoqTrackConsumer cannot express a consuming conversion, so it commits on the first group read: recv_group commits to arrival order, next_group/read_frame to sequence order, and the other order returns Unsupported from then on; datagrams are a separate cursor on either handle and never commit. js/net's Subscriber commits the same way (the first recvGroup() pins arrival order and ordered() throws afterwards), and its frame helpers (readFrame/readString/readJson) now acquire groups through the same sequence cursor as nextGroup, so frames never run backwards and endAt applies to them like every other read. The JS snapshot consumer collapses a retained backlog to the newest group, mirroring Rust.

Wire behavior changes

  • Group order within a track is now fixed newest-first, and there is no field to invert it. Previously a subscriber could set Subscriber Ordered = 1 and the publisher would rank that subscription's queued groups oldest-first (lite::Priority genuinely scheduled on it, contrary to the design note). A peer that still sets the byte now gets newest-first delivery. Under congestion this sheds the backlog rather than the live edge; a subscriber wanting sequence order reorders locally, which costs the network nothing and does not let one subscriber's preference reach a track a relay is fanning out to many.
  • The byte is removed from lite-06-wip only. Deployed lite-03/04/05 keep it in their layout, written as 0 and ignored on read, so every field behind it stays where a deployed peer expects. A lite-06 peer therefore cannot parse a lite-05 peer's SUBSCRIBE, which is normal for an in-progress draft.
  • Timestamp age now runs from a group's reach to the newest frame of the latest group, rather than first-against-first, and an age equal to Max Age expires it. Both ends apply this, so a publisher keeps a group on the wire it previously reset and a subscriber keeps one it previously discarded. A consequence worth stating plainly: a group whose successor has not yet presented a frame is never expired, because nothing proves where it ends — so a two-group track expires nothing, and the retention cache is what eventually reclaims a chain that stalls. draft-lcurley-moq-lite's Expiration section and changelog are rewritten around this.
  • The IETF adapter only ever spoke GroupOrder::Descending, so it maps to the fixed order unchanged.
  • Drafts updated in the same PR: draft-lcurley-moq-lite loses Subscriber Ordered / Publisher Ordered and gains a normative Group Order section with a changelog entry; draft-lcurley-moq-hang's recording Track Object drops the field it inherited from TRACK_INFO.

Root cause of the one behavioral test change

goaway_cluster's cluster_diamond_goaway_seamless_failover asked for ordered: true to get sequence-order arrival through every hop, then asserted "every group exactly once, in order". With the field gone the wire no longer promises that: the post-drain phase publishes three groups back to back, newest-first opens group 23's stream before 22's, and a sequence cursor legitimately seeks past 22. The test now asserts what a failover actually owes — every group arrives exactly once with its frames intact — collected in arrival order and checked for completeness per phase.

Test plan

  • just check and just test (full Rust selection, all JS packages, 52 Python tests) pass, including after merging origin/dev (feat(net)!: resolve the subscribe start from max age, with Group Start as an absolute floor #3158's Group Start floor semantics included; its budget-clamp test re-spaced for the reach rule).
  • just drafts check passes.
  • New/changed tests:
    • next_group_bursts_a_stale_backlog (rs) and the latency budget skips a buffered timeline only on the arrival cursor (js): the ordered cursor delivers a stale backlog in full while the arrival cursor still sheds it.
    • recv_datagram_leaves_the_ordered_cursor_alone (rs + js): a datagram no longer consumes a group sequence.
    • ordered_and_arrival_cursors_are_independent (rs) / the ordered and arrival cursors are independent (js).
    • subscribe_drops_the_retired_ordered_byte_on_lite06: lite-05 keeps the zero byte, lite-06 is the same message with it spliced out.
    • a_long_group_is_not_stale_while_its_tail_reaches_the_edge / a_long_group_is_stale_once_its_successor_falls_behind (rs + js): the reach rule in both directions.
    • Six existing tests moved to the new semantics. They share one shape: convicting a group needs a group beyond its successor, since a two-group track's older group has its reach at the edge.
    • ordered_carries_datagrams (rs) / the ordered handle carries datagrams (js).
    • raw_track_next_group_is_repeatable (moq-ffi): regression for the cursor conversion running more than once. The first draft of TrackInner::ordered ran mem::replace unconditionally, so the second next_group() dropped the converted handle and hit unreachable!(), aborting the Python suite.
    • next_group_bursts_a_stale_backlog_like_a_plain_track (rs, resume): plain-vs-spliced parity for the burst contract.
    • next_group_cap_holds_a_reordered_group_without_losing_late_arrivals (rs, resume): a beyond-cap group polled early must not advance the spliced cursor past a late in-range arrival.
    • raw_track_group_order_commits_on_first_read (moq-ffi) and the first group read commits the cursor and refuses the other order / recvDatagram does not commit the group cursor (js): both commitment directions, plus datagrams never committing.
    • ordered frame reads drain a stale backlog / ordered frame reads skip a late lower-sequence group / endAt caps frame-level reads like the group cursor (js): the frame helpers ride the sequence cursor.
    • a_stamped_successor_expires_an_unstamped_group (rs) / a stamped successor expires a group stalled before its first frame (js): the reach rule covers unstamped candidates, which is what let the wall-clock measure go.
    • retention eviction surfaces as a gap for a handed-out group (js): wall-clock reclamation is the retention layer's, and reads as Lagged.

Cross-Package Sync

Skipped rows, deliberately: the generated swift/Sources/MoqFFI and kt/moq-ffi bindings still need regenerating against the new moq-ffi surface. They compile as-is (neither carried the removed field), so this lands with the stacked bindings PR along with the remaining doc/lib/{swift,kt,go} pages.

(Written by Claude Opus 5)

…dered wire field

`track::Subscriber` exposed two delivery orders as two methods on one handle, with
coupled cursors: `recv_datagram` bumped the sequence cursor, and `read_frame` bumped
both. Interleaving them was expressible and the result was hard to reason about, so the
rule lived in doc comments rather than in the type.

`Subscriber::ordered(self) -> track::Ordered` makes the order a handle. Consuming `self`
makes mixing unrepresentable. The plain `Subscriber` keeps the arrival cursor
(`recv_group`) and datagrams (`recv_datagram`), which is what the relay forwarders use;
`Ordered` owns `next_group` plus its own bounds, control, and finished. Datagrams no
longer touch any group cursor: they are unordered by construction, so there is nothing
for a sequence cursor to do with them.

`read_frame` is removed from the track level. It was two different functions under one
name (plain: one frame per group then skip; spliced: drain every frame in sequence), so
it is removed rather than moved. Callers loop `next_group()` + `group.read_frame()`.

The ordered cursor no longer discards. A group already cached costs nothing to deliver,
and dropping it puts a hole in the sequence a decoder is reading, so a backlog is
delivered as a burst, in order, however old it is. `Subscription::max_age` instead bounds
how long a *handed-out* group may block, which group expiry already enforces (it only
convicts a read that is Pending). This matches what moq-mux's container consumer has
always done. The arrival cursor keeps writing a backlog off, which is what lets a relay
shed one.

`Ordered` is a `bool` on the wire and in the model, and it goes away entirely: model,
`Subscription`, `Info`, aggregation, the lite priority queue, and the lite-06-wip wire.
Group order within a track is now normatively newest-first, so a congested track sheds
its backlog rather than its live edge; a subscriber wanting sequence order reorders
locally, which costs the network nothing and does not let one subscriber's preference
reach a track a relay is fanning out to many. Deployed lite-03/04/05 keep the byte in
their layout (written as 0, ignored on read) so every field behind it stays put. The IETF
adapter already only spoke Descending, so it maps to the fixed order unchanged.

moq-ffi's `MoqTrackConsumer` cannot express a consuming conversion, so it commits on
first use: whichever cursor a caller reaches for first is the one the track keeps, and
the other returns `Unsupported`. `moq_track_info.ordered` and `moq_subscription.ordered`
are removed from the C ABI, which is a layout break for a prebuilt consumer.

`goaway_cluster`'s failover test asked for `ordered: true` to get sequence-order arrival
across every hop. With the field gone the wire no longer promises that, so it now asserts
what a failover actually owes: every group arrives exactly once with its frames intact,
collected in arrival order and checked for completeness per phase.

Regression test: `raw_track_next_group_is_repeatable` covers the FFI cursor conversion
running more than once (the first draft dropped the converted handle on the second call).

Drafts updated in the same PR: `draft-lcurley-moq-lite` loses `Subscriber Ordered` and
`Publisher Ordered` and gains a normative Group Order section;
`draft-lcurley-moq-hang`'s recording Track Object drops the field it inherited.

The generated `swift/`/`kt/` bindings still need regenerating against the new moq-ffi
surface; that lands with the stacked bindings PR.

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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d4a746106f

ℹ️ 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".

Comment on lines +3275 to +3276
pub fn poll_next_group(&mut self, waiter: &kio::Waiter) -> Poll<Result<Option<group::Consumer>>> {
self.inner.poll_next_group(waiter)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Preserve cached groups for spliced ordered tracks

For any track reached through an origin's route-fed broadcast, this delegates Ordered to resume::Subscriber::poll_next_group, which obtains groups through poll_recv_group; the active per-segment subscriber therefore still applies Subscription::max_age and discards a buffered backlog. With the default zero budget, an ordered lossless consumer such as moq-json can lose all cached records except the live edge, even though the new Ordered contract promises to burst every already-servable group. The spliced path needs a sequence cursor that bypasses the pre-handoff staleness filter just like PlainSubscriber::poll_next_group.

Useful? React with 👍 / 👎.

Comment thread rs/moq-transcode/src/feed.rs Outdated
Comment on lines +177 to +179
// The feed serves whichever rungs are active, so there is no single
// downstream subscription to mirror; live-edge defaults fit every rung.
let mut subscriber = inner.source.subscribe(None).await?;
let mut subscriber = inner.source.subscribe(None).await?.ordered();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Keep the transcoder subscription at the live edge

When the first rung activates after the source has accumulated cached groups, converting this subscription to Ordered now replays the complete retained backlog because ordered selection deliberately no longer applies the zero max_age budget. That contradicts the live-edge requirement immediately above and makes a real-time transcode decode and republish stale video before catching up. This feed should use the arrival cursor's recv_group behavior, or explicitly position the ordered cursor at the current live edge.

Useful? React with 👍 / 👎.

Comment thread rs/moq-ffi/src/consumer.rs Outdated
Comment on lines +396 to +400
if matches!(self.track, Cursor::Arrival(_)) {
let Cursor::Arrival(track) = std::mem::replace(&mut self.track, Cursor::Converting) else {
unreachable!("just matched Arrival");
};
self.track = Cursor::Ordered(track.ordered());

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Commit the FFI cursor on the first arrival read

If a foreign caller first uses recv_group or recv_datagram, the cursor remains represented as Cursor::Arrival, so a later next_group still converts it instead of returning Unsupported as the stated first-use contract requires. On a plain/local track, recv_group followed by next_group can consequently return the already-consumed cached group again, after which further arrival reads unexpectedly fail. Record an arrival-committed state when the first arrival operation runs and refuse conversion from that state.

Useful? React with 👍 / 👎.

`is_stale` judged a group that had not yet been handed out by its *first* frame
timestamp. A group covering a long span was therefore convicted the moment it opened,
even while its tail sat level with the live edge and every frame in it was unread: a
group spanning 0..2000ms with the edge at 2000ms was dropped under a 500ms budget,
which is exactly the group being filled.

Measure the candidate by its newest frame instead, against the edge group's first. The
comparison then reads as "where this group's content ends, versus where newer content
begins", which is the age that was always intended. A group already handed out was
already correct (measured at its reader's position, per `group::Position`), so only the
hand-out check moves. Single-frame groups have first == latest and are unaffected, which
is why the rest of the suite is untouched.

This is the normative measure both ends apply, so `draft-lcurley-moq-lite`'s Expiration
section and changelog change with it.

Also give `track::Ordered` the datagram channel. Datagrams are unordered by construction
and live on a separate cursor, so the choice of group order says nothing about them;
withholding them only forced a track using both channels to subscribe twice.

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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 50fcd3bbd7

ℹ️ 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".

Comment thread js/net/src/track.ts
* Groups outside the subscription's `maxAge` budget are discarded before reading.
*/
async readFrameSequence(): Promise<({ group: number; frame: number } & Frame) | undefined> {
async #readFrameSequence(): Promise<({ group: number; frame: number } & Frame) | undefined> {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Preserve cached groups in ordered frame helpers

When Ordered.readFrame() or readFrameSequence() is called with multiple buffered groups, this helper still applies the old maxAge stale-filter loop at lines 1034-1042. With the default zero budget, every group before the live edge is removed before a frame is returned, even though the new Ordered contract promises to burst every already-servable group. A lossless one-frame-per-group log using these convenience methods therefore silently loses its cached backlog; ordered frame reads should bypass selection-time staleness just like nextGroup(). (Written by GPT-5.6 Sol)

Useful? React with 👍 / 👎.


constructor(track: Moq.Track.Subscriber, config: Config<T> = {}) {
this.#track = track;
this.#track = track.ordered();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Collapse snapshot consumers to the newest retained group

When a snapshot producer rolls into two or more retained groups before this JS consumer reads, converting the track to Ordered makes nextGroup() return the oldest group, while next() drains only that group's frames before yielding. The caller consequently observes one obsolete snapshot per retained group instead of only the latest value; unlike this implementation, the Rust snapshot consumer repeatedly polls groups to the newest one before yielding. Drain all immediately available groups and reconstruct only the newest group. js/CLAUDE.mdL20-L22 (Written by GPT-5.6 Sol)

Useful? React with 👍 / 👎.

Comment thread rs/moq-ffi/src/consumer.rs Outdated

async fn recv_datagram(&mut self) -> Result<Option<MoqDatagram>, MoqError> {
let Some(datagram) = self.track.recv_datagram().await? else {
let Some(datagram) = self.arrival()?.recv_datagram().await? else {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Dispatch datagrams through the committed FFI cursor

After a caller first uses next_group or read_frame, the cursor is converted to Cursor::Ordered, so this arrival() call makes every later recv_datagram fail with Unsupported. The new core moq_net::track::Ordered::recv_datagram method explicitly keeps datagrams available after conversion for tracks carrying both channels, but this bridge makes that combination impossible from Python, Swift, Kotlin, or Go; dispatch the read through whichever cursor is committed. (Written by GPT-5.6 Sol)

AGENTS.md reference: AGENTS.md:L71-L73

Useful? React with 👍 / 👎.

Comment on lines 46 to +47
/// Construct with the fields you care about; the rest default to moq-net's defaults
/// (priority 0, unordered, no staleness tolerance, full group range).
/// (priority 0, no staleness tolerance, full group range).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Remove the retired Ordered field from the Go guide

Removing ordered from MoqSubscription also removes Ordered from the generated Go Subscription, but doc/lib/go/moq.md still constructs moq.Subscription{Priority: 10, Ordered: true} and updates it with Ordered: false at lines 334 and 348, then documents the retired behavior. Those published examples no longer compile, so update the Go guide alongside the other binding documentation. (Written by GPT-5.6 Sol)

AGENTS.md reference: AGENTS.md:L186-L190

Useful? React with 👍 / 👎.

Max age judged a group by where it *started*, so a group was convicted for being
behind rather than for having nothing left to give. That is the wrong axis. Priority
already transmits newer groups first, so an older group consumes only leftover
capacity and closes the gap faster than the live edge advances: being behind is
survivable, and a receiver that is behind converges without losing content. What
cannot be recovered is a group with nothing left worth delivering.

Measure a group by how far it could still *reach* instead: the first frame timestamp
of its successor, since a group cannot present past where the next group begins. Its
own frames prove nothing, because frame durations are not on the wire and a group's
last timestamp is where that frame starts, not where the group ends. A group whose
successor has not arrived is therefore never expired on timestamp age; the wall-clock
measure still backstops it.

Reach is an exclusive bound, so the comparison is `>=`: the freshest frame a group
could still hold sits strictly below its reach, and an age equal to the budget already
puts every frame in it past the budget. That also makes a zero budget fall out of the
general rule, so `wall_stale` loses its `budget.is_zero()` special case.

This retires all three competing measures at once: the original first-frame one, the
reader-position preference from #2890 (a drained-but-open group measured as level with
the edge, so nothing could convict a stall), and the newest-frame one from the previous
commit (which had the same defect and hung a reader outright).

`Edge` carries a suffix minimum over stamped groups, built in the scan `live_edge`
already did, so walking a backlog stays linear rather than rescanning per candidate. It
is `Clone` rather than `Copy` now, so `is_stale` and `poll_stale` take it by reference.
Once `lookup` becomes a `BTreeMap` (it already is on main, via #3088) this collapses to
a `range(sequence + 1..)` walk and the table goes away.

Six tests move to the new semantics. The shape they share: a two-group track can never
expire the older group on timestamp age, because its reach *is* the edge. Convicting it
needs a group beyond the successor, which is what each test now sets up.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 28, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-08-29T03:09:43.399875Z 4a147f5 New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4f5fde1337

ℹ️ 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".

Comment thread js/net/src/track.ts Outdated
Comment on lines +1186 to +1188
ordered(): Ordered {
if (this.#committed) throw new Error("track is already read in sequence order");
this.#committed = true;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject ordering after an arrival read

When a caller has already consumed a group with recvGroup(), this check still permits ordered(). Because arrival reads do not advance #nextSequence, a lower-sequence group that arrives afterward is then returned by Ordered.nextGroup(), so the same subscription can yield 5 followed by 3 despite the new API promising one consistent cursor choice. Record that the arrival cursor has been used and reject conversion after that point. (Written by GPT-5.6 Sol)

AGENTS.md reference: AGENTS.md:L157-L161

Useful? React with 👍 / 👎.

`live_edge` collapsed the stamped groups into a suffix minimum, so a candidate's reach
became the earliest timestamp among *all* later groups rather than where its own
successor begins. Timestamps need not rise with sequence: a rewind can put a much
earlier timestamp on a much later group. With starts of 10s, 1s and 2s on sequences 1,
2 and 3, group 0's reach resolved to 1s instead of its successor's 10s, so a 500ms
budget discarded a group that could still hold frames through nearly 10s.

A minimum shrinks the bound, which is the unsafe direction: the bound exists to prove a
group has nothing left worth delivering, and only its immediate successor proves
anything about where it ends. A later group starting earlier says nothing at all. The
draft already specified the next group by sequence number; the code disagreed with it.

Dropping the suffix pass is a simplification: sorting by sequence already puts the
immediate successor one lookup away.

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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c535fb9bf0

ℹ️ 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".

Comment thread js/net/src/track.ts Outdated
*/
ordered(): Ordered {
if (this.#committed) throw new Error("track is already read in sequence order");
this.#committed = true;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Invalidate pending arrival reads when ordering

If recvGroup() is already waiting when ordered() is called, setting #committed does not make the old handle inert: recvGroup() checks #live() only before its loop, then resumes after Signal.race and consumes the next group. The returned Ordered handle can consequently miss that group and remain pending, violating the one-cursor contract. Recheck the committed state after each wake or cancel outstanding arrival reads during conversion. (Written by GPT-5.6 Sol)

AGENTS.md reference: AGENTS.md:L157-L161

Useful? React with 👍 / 👎.

kixelated and others added 2 commits August 28, 2026 16:00
…mit cursors on first read

The spliced subscriber's sequence path filtered the arrival path, which prunes
by max age, so the same Ordered handle burst a backlog on a plain track and
discarded it across a takeover. Each segment now drives its own sequence
cursor with one group staged per segment, and the lowest sequence across
segments is delivered next.

js/net's Subscriber never committed on an arrival read, so recvGroup() then
ordered() interleaved both cursors on one buffer. The first group read now
commits the track: the other order throws. moq-ffi grows the matching
Uncommitted state, refuses mixing with Unsupported instead of converting
after arrival reads, and dispatches datagrams through either handle without
committing. moq-transcode's shared decode feed returns to arrival order so
its drift budget keeps dropping what falls behind the live edge instead of
bursting the backlog through the codecs.

Also reconciles the dev merge: the reader-position expiry plumbing dev
carried (group::Position.presentation) is deleted since the reach rule
retired that measure, and the group bench's sequence arm drives
track::Ordered.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dae5cc6d13

ℹ️ 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".

Comment on lines +439 to +441
let datagram = match &mut self.track {
Cursor::Uncommitted(track) | Cursor::Arrival(track) => track.recv_datagram().await?,
Cursor::Ordered(track) => track.recv_datagram().await?,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Let datagrams bypass a pending group read

If a foreign caller starts next_group or recv_group while no group is available and concurrently calls recv_datagram, the group operation holds the Task<TrackInner> mutex for its entire await, so this new datagram dispatch cannot run until a group arrives. A datagram does not wake the group cursor and may expire while queued, making the advertised independent group/datagram channels unusable concurrently from Python, Swift, Kotlin, or Go. Drive the datagram cursor without waiting for the group operation's task lock. (Written by GPT-5.6 Sol)

AGENTS.md reference: AGENTS.md:L157-L159

Useful? React with 👍 / 👎.

kixelated and others added 3 commits August 28, 2026 19:11
Codex's adversarial pass found two holes in the takeover commit. The spliced
sequence path left each segment's inner cursor uncapped, so a beyond-cap group
arriving early advanced it past an in-range group still on the wire, losing
that group for good once the cap rose; the inner cursor now carries the outer
cap. js/net's frame helpers read the lowest buffered group with no floor, so a
late lower-sequence group was fed to the caller backwards, and they still
discarded stale groups the Ordered contract promises to deliver; they now
acquire groups through the same sequence cursor as nextGroup, which also makes
endAt apply to them like every other read.

The JS snapshot consumer replayed every retained group where the Rust one
collapses to the newest, so a late joiner decoded each superseded state; it
now jumps the cursor to the latest buffered group.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-ordered-handle

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…the cache

Expiration and reclamation were two rules under one budget. Expiration is now
timestamp-only: a group is stale once its reach (its successor's first frame
timestamp) falls a full budget behind the newest frame of the latest group.
The candidate needs no timestamp of its own, so an empty or stalled group is
bounded by its stamped successor the same way, which is what the retired
wall-clock arm existed for. Wall-clock reclamation of idle content belongs to
the retention cache (Publisher Max Age, the pool's access-time expiry), whose
symptom is a gap (Lagged), not a budget verdict.

Removes the wall edge, the per-slot arrival instants, and the per-frame
activity clocks from both implementations, along with the group Position
plumbing that carried them into every guarded read. The draft's Expiration
section drops the wall-clock measure to match.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 135d34ef9d

ℹ️ 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".

Comment on lines +42 to +43
const latest = this.#track.latest();
if (latest !== undefined) this.#track.startAt(latest);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Recheck the newest group before draining a snapshot

When the consumer already holds an older group and the producer rolls to a newer snapshot group while unread deltas remain buffered in the old one, this branch is skipped and lines 54-58 yield the obsolete state before advancing to the new snapshot. Fresh evidence after the earlier review is that the new latest()/startAt() fix runs only inside if (!this.#group), whereas the Rust consumer checks poll_next_group on every poll, so an active JS consumer still violates the documented collapse-to-head behavior across a group roll. (Written by GPT-5.6 Sol)

Useful? React with 👍 / 👎.

A group is never expired until at least the next group (by sequence number) has been received or queued.
Once a newer group exists, the group's age is measured two ways, and it is expired once **either** measure exceeds the relevant `Max Age`:
A group is never expired until a later group (by sequence number) has presented a frame.
Once one has, the group's **timestamp age** is the difference between the *newest* frame timestamp of the latest group that has at least one frame, and this group's **reach**, defined below; the group is expired once that age meets the relevant `Max Age`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Use reach consistently when resolving subscription starts

This changes expiration to measure age from a group's successor-defined reach and expire when the age meets the budget, but the same draft still says at line 504 that expiration requires the age to exceed the budget and, more importantly, tells publishers at line 996 to resolve Group Start using the group's first frame. Implementations following the SUBSCRIBE section can therefore select and deliver a different oldest group than implementations following this section and the Rust/JS code, defeating the promise at line 998 that start selection and expiration cannot disagree. (Written by GPT-5.6 Sol)

AGENTS.md reference: AGENTS.md:L211-L211

Useful? React with 👍 / 👎.

Comment on lines +3163 to +3164
pub fn ordered(self) -> Ordered {
Ordered { inner: self }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Retarget this breaking API change to dev

This commit is based directly on d8f9607 (main) while removing published Rust and JS fields and methods such as Info::ordered, Subscription::ordered, and with_ordered; downstream consumers using those APIs stop compiling. The repository explicitly reserves dev for renamed, removed, or signature-changed public items, so this breaking handle conversion needs to target that branch rather than land on main. (Written by GPT-5.6 Sol)

AGENTS.md reference: AGENTS.md:L217-L219

Useful? React with 👍 / 👎.

Comment thread js/net/src/track.ts
Comment on lines +1263 to +1265
/**
* Read the next frame across groups, discarding older groups.
* Treat the returned frame bytes as read-only; they are shared with other consumers.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Document that ordered frame reads drain the backlog

The new Ordered.readFrame() implementation acquires groups through the sequence cursor and drains every already-servable group, but this public API comment still promises that it discards older groups. A consumer choosing this convenience method for latest-frame behavior will instead replay the retained backlog, so the generated API documentation should describe the ordered burst semantics consistently with the class contract and implementation. (Written by GPT-5.6 Sol)

AGENTS.md reference: AGENTS.md:L115-L117

Useful? React with 👍 / 👎.

… in js

Codex's re-review caught the staging design's flaw: staging a group advances a
segment's inner cursor before the outer cursor commits, so a cap lowered after
a lookahead stranded a late in-range arrival the inner cursor had stepped
past. The spliced sequence path now seeks each segment's cache for the lowest
group at the outer floor without consuming anything, delivers the winner, and
advances only its own floor; the staged slot and inner cap tracking are gone.

js/net's nextGroup now abandons the group the frame helpers were draining when
it moves past it, so interleaving the two read styles on one Ordered handle
can no longer replay older frames behind the cursor.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4a147f5a8f

ℹ️ 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".

Comment on lines +2727 to +2728
let index = self.suffix.partition_point(|(seq, _)| *seq <= sequence);
self.suffix.get(index).map(|(_, timestamp)| *timestamp)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Treat an unstamped immediate successor as unbounded

When group N+1 exists but has not presented a frame while N+2 has, suffix omits N+1 and this lookup incorrectly uses N+2's timestamp as N's reach. Because timestamps need not increase with sequence, N+1 may later start much closer to the live edge, so is_stale can prematurely abort N and silently create a gap even though its actual reach is still unknown. Keep an unstamped successor sentinel, or explicitly return None unless the immediate successor is stamped; the same stamped-only lookup in js/net/src/track.ts needs the matching fix so both implementations follow the draft's unbounded-reach rule.

AGENTS.md reference: AGENTS.md:L211-L211

Useful? React with 👍 / 👎.

@kixelated
kixelated merged commit 0fbb765 into dev Aug 29, 2026
9 checks passed
@kixelated
kixelated deleted the claude/net-ordered-handle branch August 29, 2026 04:27
kixelated added a commit that referenced this pull request Aug 29, 2026
The json-cut tests (#3146) and the ordered-handle rework (#3099) merged past
each other with cancelled CI, so dev's cut tests still call the pre-handle
structure(track.subscribe(...)) shape, which no longer typechecks or runs.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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