refactor(net)!: carry subscription bounds as positions - #2569
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @kixelated, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
![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
moq/rs/moq-net/src/model/subscription.rs
Lines 194 to 197 in b05dedf
Represent the initial position's missing predecessor
When a Rust caller uses the documented half-open representation for an empty range, such as start = end = Some(Position::group(0)), before() produces { group: 0, frame: u64::MAX }, which is greater than the input rather than strictly below it. The new libmoq conversion consequently caps the local cursor at group 0, while WireBounds similarly serializes this endpoint as including group 0, so a range that should deliver nothing can deliver that group. Because Position and Subscription::end are now public, the claim that nothing produces this value is unenforceable; represent the absence of a predecessor or reject empty ranges instead of saturating. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L122-L126
ℹ️ 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".
| pub fn with_group_end(mut self, group: impl Into<Option<u64>>) -> Self { | ||
| // Saturating so a `u64::MAX` group cannot wrap to an empty range. Unreachable | ||
| // from the wire, where varints cap at 2^62-1. | ||
| self.end = group.into().map(|group| Position::group(group.saturating_add(1))); |
There was a problem hiding this comment.
When a local caller requests with_group_end(u64::MAX), saturating_add(1) leaves the exclusive endpoint at Position::group(u64::MAX), so group u64::MAX itself is excluded despite the builder promising to include it; with_end(group, u64::MAX) has the analogous last-frame loss. These are valid model inputs even though the wire uses smaller varints. Return an error or use an endpoint representation that can express the successor rather than silently narrowing the requested range. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L122-L126
Useful? React with 👍 / 👎.
`Subscription` exposed `group_start` / `frame_start` / `group_end` / `frame_end` as four independently-settable public fields while the model worked in whole `Position`s, so `set_start` / `set_end` / `start()` / `end()` existed purely to adapt between the two shapes. Assigning a frame without its group stayed expressible, and was caught only by the remote decoder. `start` and `end` are now `Option<Position>` and the adapter layer is gone. The end is exclusive, like `std::ops::Range`. That is what lets one field carry both "through the end of group 5" and "up to frame 2 of group 5"; an inclusive end needs a sentinel frame for the first, which is what `end()` was faking with `u64::MAX`. The aggregate ordering then falls out for free, since group 6's head outsorts every frame of group 5. The wire already agreed: `Group End` and `Frame End` are both `absolute + 1`. The builders are exclusive too, rather than translating. `with_group_start` and `with_group_end` are replaced by `with_start` and `with_end` taking a `Position`, so what you set is what you read back. `Position::after` and `Position::after_group` build one from the inclusive bound a caller usually holds, which puts the `+ 1` in one named place and makes the convention visible at the call site. That was the real defect in `with_group_end(5)`: not that it was inclusive, but that nothing in the name said so. `resume::slice` loses its inclusive/exclusive conversion entirely, because segment bounds were already half-open. The wire conversion collapses into two named inverses, `Bounds::positions` and `WireBounds::new`, rather than being spelled out field by field at each call site. `moq-ffi` and `libmoq` keep their own inclusive `group_start` / `group_end` in their binding structs, which suits those audiences; only the conversion into `Subscription` moved, so no bindings regenerate and the C docs stay accurate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
b05dedf to
ac2367a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac2367af92
ℹ️ 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".
| // An exclusive end at the head of a group means the group below it is the | ||
| // last one, and it is served whole. Saturating covers an end of (0, 0), | ||
| // which would serve nothing and which nothing produces. | ||
| Some(end) if end.frame == 0 => (Some(end.group.saturating_sub(1)), None), |
There was a problem hiding this comment.
When a Rust caller requests the valid half-open range start = Position::group(0), end = Position::group(0), the end denotes an empty range, but saturating_sub(1) converts it to the inclusive wire bound end_group = 0. The peer consequently serves group 0 instead of serving nothing. Reject or otherwise represent this boundary rather than silently widening the subscription. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L122-L126
Useful? React with 👍 / 👎.
The wire has no encoding for an empty range: `Group End` = 0 already means unbounded. `WireBounds::new` floored an exclusive end of `(0, 0)` with `saturating_sub`, which put `end_group` = 0 on the wire, meaning "through group 0" and delivering the single group the caller excluded. Reachable only since the builders started taking a `Position` directly, which is also what made `Position::group(0)` expressible as an end. Every internal bound sits at or above the first frame, so nothing in the model produces it. An empty range is now dropped as no demand at all, which is what "serve me nothing" means: nothing opens on establish, and a live subscription is canceled on update. The aggregate can only be empty when every downstream subscriber wants nothing, since an unbounded one absorbs the rest, so canceling upstream is correct. The saturating floor stays for totality but carries a `debug_assert` for the precondition rather than a comment claiming it cannot happen. Found by an adversarial review pass. Both regression tests fail with the filter removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6803521eb
ℹ️ 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 last position strictly below this one, turning an exclusive bound such as | ||
| /// [`Subscription::end`] into the inclusive one an API like | ||
| /// [`crate::track::Subscriber::end_at`] wants. |
There was a problem hiding this comment.
before reject the first position
When an external caller invokes Position::group(0).before(), saturation returns Position { group: 0, frame: u64::MAX }, which sorts after the input despite this public method promising the last position strictly below it. The new empty-range filter in lite/subscriber.rs only protects wire encoding and does not make this now-public conversion correct, so consumers converting an exclusive empty end into an inclusive bound can accidentally include group 0. Return Option/Result or otherwise make this boundary unrepresentable instead of returning a contradictory position. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L122-L126
Useful? React with 👍 / 👎.
`Position::after`, `after_group` and `before` saturated at the ends of the
`u64 x u64` lattice, each returning a value that contradicted its own contract:
- `after_group(u64::MAX)` left the exclusive end at `group(u64::MAX)`, excluding
the group the caller asked to include.
- `after(g, u64::MAX)` returned `{g, u64::MAX}`, dropping the last frame instead
of rolling to the next group.
- `group(0).before()` returned `{0, u64::MAX}`, which sorts *above* its input
while the method promises the last position strictly below it. Reachable
through `libmoq`, whose local read cursor bypasses the wire-side empty-range
filter.
All three now return `Option`. Past the last group has no position, and
`Subscription::end` already spells unbounded as `None`, so the two meanings
coincide: `with_end(Position::after_group(u64::MAX))` is unbounded and includes
the group rather than dropping it. Past the last frame of a group is the head of
the next one. Below the first position there is nothing, which is the empty
range.
`None` reads as "no cap" at both `before` call sites, so neither propagates it
blindly: `resume` asserts the segment case is unreachable and notes that
`Segment::covers` filters on the exclusive bound regardless, and `libmoq` caps
at the first group rather than leaving the cursor open.
Reported by the Codex connector on the PR.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fd0883cd2
ℹ️ 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".
| // can say is "through group 0", which would deliver the one group the caller | ||
| // excluded. No demand at all is the faithful translation, and it is reachable | ||
| // only from a caller: every internal bound sits at or above the first frame. | ||
| let pref = pref.filter(|sub| sub.end != Some(Position::group(0))); |
There was a problem hiding this comment.
When a caller requests an empty range at a nonzero position, such as start = Position::group(5), end = Position::group(5), this filter retains it because it only recognizes an end at group 0. WireBounds then emits start_group = 5, end_group = 4; the publisher applies those as a start cursor and a pausing cap, so the subscription remains live indefinitely and counts as upstream demand despite requesting nothing. Fresh code now special-cases only the previously reported group-0 boundary, so treat every explicit start >= end as no demand and ensure such ranges do not widen the aggregate. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L98-L99
Useful? React with 👍 / 👎.
`match Some/None` mapping one arm is `Option::map`, which clippy rejects under `-D warnings`. Caught by CI rather than locally: `just fix` runs `cargo clippy --fix` and the `just check` that followed it in the same shell reused that cached result instead of re-linting the crate. Running clippy on its own reproduces it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The #2569 position refactor collided with the SUBSCRIBE_START floor tracking in establish and update. Resolved onto the new shape, and reshaped the floor bookkeeping per review while at it: A buffered START is ignored once an update has moved the requested start (SubStream.updated): the declaration describes establish-time demand, no fresh START follows an update, and applying it in either direction can strand readers in a range the publisher no longer serves. The min-clamp it replaces was wrong both ways: it could undo a forward update's floor, and it neutered legitimate skip-declarations above the request. Demand at the live edge (start None) now clears the floor instead of leaving a stale one installed: the edge may resolve below an old future start, and those groups must not be permanent misses. track::Producer:: start_at takes Option<u64> accordingly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
moq_net::track::Subscriptionexposed its delivery range as four independently-settable public fields (group_start/frame_start/group_end/frame_end) while the model itself worked in whole positions.set_start/set_end/start()/end()existed purely to adapt between the two shapes, and a frame index without the group it counts from stayed expressible, caught only by the remote peer's decoder asInvalidSubscribeLocation.startandendare nowOption<Position>, and the adapter layer is deleted.This is the follow-up promised in #2537, which landed the frame-precise bounds themselves and closed the builder path with
with_start/with_end. That left the field path open; this closes it.The end is exclusive
Half-open, like
std::ops::Range, in the field and in the builders. That is the load-bearing decision, so it is worth stating why.The end bound has three states, not two: unbounded, through the end of a group, and capped at a frame within a group. An inclusive end can only express the middle one with a sentinel frame, which is exactly what
end()was doing (frame_end.unwrap_or(u64::MAX)) so the "widest end wins" fold would order correctly. A plainOption<Position>would have had to either leak that sentinel to consumers or nest a secondOption, which is two fields again.Exclusive collapses all three into one uniform value:
endNoneSome(Position::group(6))Some(Position { group: 5, frame: 3 })The ordering then falls out for free: group 6's head sorts above every frame of group 5, so a whole-group subscriber absorbs a frame-capped one in the aggregate with no special case. And the wire already agreed, since
Group EndandFrame Endare both encoded asabsolute + 1.What this deletes
Subscription::{set_start, set_end, start, end}(the four-field/two-position adapter) and theu64::MAXsentinel with it.resume::slice's inclusive/exclusive conversion. Segment bounds were already half-open, so intersecting them is nowmin_some(prefs.end, end)where it wasmin_some(prefs.end(), end.map(Position::before)).SubStream's splitstart_group/start_frame, now onestart: Option<Position>.The wire conversion is now two named inverses,
Bounds::positions(decode) andWireBounds::new(encode), instead of being spelled out field by field at four call sites.widen_frame_boundsalso reads as what it actually does: rounding both bounds outward to the enclosing group.Public API changes
Breaking, hence
dev.moq_net::track::Subscription:group_start/frame_start/group_end/frame_endare replaced bystart: Option<Position>andend: Option<Position>.moq_net::track::Positionis now public (waspub(crate)), along withPosition::groupandPosition::before.with_group_startandwith_group_endare replaced bywith_startandwith_end, each takingimpl Into<Option<Position>>and matching the field exactly. Four builders become two.Position::after(group, frame)andPosition::after_group(group).The builders are exclusive rather than translating, so what you set is what you read back. An earlier revision kept them inclusive to spare call sites; that was wrong, because it meant
with_group_end(5)andsub.enddisagreed by one with nothing in the name to warn you.Position::after/after_groupbuild the exclusive bound from the inclusive one a caller usually holds, which puts the+ 1in a single named place and states the convention at the call site:rs/moq-ffiandrs/libmoqkeep their own inclusivegroup_start/group_endin their binding structs, which suits those audiences; only the conversion intoSubscriptionmoved. No bindings regenerate, anddoc/lib/c/index.mddocuments that C struct, so it stays accurate.libmoq/src/consume.rsalso read the fields directly, and now maps the position back to the group sequence its local read cursor wants.Review round 1 (adversarial)
Fixed: an empty range asked for group 0. The wire has no encoding for an empty range, since
Group End= 0 already means unbounded.WireBounds::newfloored an exclusive end of(0, 0)withsaturating_sub, puttingend_group= 0 on the wire, which means "through group 0" and delivers the single group the caller excluded.The root cause is that the model's range algebra is strictly more expressive than the wire's, and the encoder rounded to the nearest representable value instead of refusing. For the empty range, the nearest value is its opposite.
It became reachable in this PR: before the builders took a
Positiondirectly,with_group_end(g)producedg + 1andwith_end(g, f)producedframe + 1, so(0, 0)could not be built. Nothing internal produces it either, since every model bound sits at or above the first frame. So it needed a caller, andPosition::group's own doc invites one.An empty range is now dropped as no demand before it reaches the wire, which is what "serve me nothing" means: nothing opens on establish, and a live subscription is canceled on update. The aggregate can only be empty when every downstream subscriber wants nothing, since an unbounded one absorbs the rest, so canceling upstream is right. Regression tests
an_empty_range_opens_no_subscriptionandan_empty_range_cancels_a_live_subscriptionboth fail with the filter removed.Fixed: the position conversions saturated at both extremes.
after,after_groupandbeforeeach returned a value that contradicted its own contract at the ends of theu64 x u64lattice:after_group(u64::MAX)excluded the group it promised to include,after(g, u64::MAX)dropped the last frame instead of rolling to the next group, andgroup(0).before()returned a position sorting above its input. The last was reachable throughlibmoq, whose local read cursor bypasses the wire-side filter above.All three return
Optionnow. Past the last group has no position, andSubscription::endalready spells unbounded asNone, so the two meanings coincide andwith_end(Position::after_group(u64::MAX))correctly includes the group.Nonereads as "no cap" at bothbeforecall sites, so neither propagates it blindly. Pinned bypositions_are_total_at_the_extremes.Cross-Package Sync
No row applies. The wire bytes are unchanged: this moves the Rust type that feeds the codec, not the codec.
drafts/,js/net, anddoc/are untouched on purpose rather than by omission.Test plan
nix develop --command just check— clean, includingcargo doc -D warnings, which caught four intra-doc links to the renamed fields that compilation alone missed.nix develop --command just rs test— 2391 tests run, 2391 passed. (nextest flagsmoq-relay config::tests::cli_does_not_clobber_toml_tiersas leaky; it is unrelated, andmoq-relayis untouched here.)nix develop --command just rs loom— passes (9 + 5 model checks, no deadlock or leakedArc).rs/CLAUDE.mdmakes this a manual gate for changes undermoq-net/src/model/.New coverage for the exclusive seam, which is where an off-by-one would silently drop or duplicate a frame at every relay hop:
wire_bounds_convert_the_exclusive_endandwire_bounds_match_the_builderson the encode side,bounds_convert_to_positionson the decode side (including a frame bound arriving without its group, which now cannot reach the model at all).a_frame_bound_without_its_group_is_droppedfrom #2537 is removed rather than ported. It asserted that a frame with no group is dropped on encode; that state is now unrepresentable, so its input is a legitimate position and the assertion had inverted. Deleting a test because the type system subsumed it is the point of the change.(Written by Opus 5)