Skip to content

feat(net): start and end subscriptions and fetches at a specific frame - #2537

Open
kixelated wants to merge 1 commit into
mainfrom
claude/frame-specific-subs-fetches-6d6a0b
Open

feat(net): start and end subscriptions and fetches at a specific frame#2537
kixelated wants to merge 1 commit into
mainfrom
claude/frame-specific-subs-fetches-6d6a0b

Conversation

@kixelated

@kixelated kixelated commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Closes #2517.

Summary

Route migration splices per-session tracks at group boundaries, so it never resumes a track whose current group can stay open indefinitely. moq-json::stream keeps its whole append log in one group, and a quiet catalog may not roll for a long time. A route dying partway through such a group stalled the logical subscriber until a group that might never come.

Bounds are frame-precise now, end to end.

The governing rule is that a partial group only ever goes to a subscriber that asked for one. A publisher that cannot serve a group from the frame the subscription names skips it and resolves to a later group, rather than delivering the part it holds. A group is the unit of decodability: dropping leading groups leaves a stream the application can still decode, whereas dropping leading frames often does not, whether because the group opens with a keyframe the rest depends on or because its compression state lives in the frames that went missing. Only the subscriber knows whether a partial group is any use to it. That also keeps every subscriber which does not opt in on exactly the pre-lite-06 behavior.

Wire (moq-lite-06)

  • SUBSCRIBE / SUBSCRIBE_UPDATE carry Frame Start and Frame End, qualifying the start and end group. Frame Start is a plain index; Frame End is the index + 1, matching Group End. Two abutting subscriptions therefore carry the same numbers on the wire, which is what makes a splice exact.
  • FETCH carries the same pair, bounding the returned frames within the group. A publisher that cannot serve the range in full resets, since the response has no header to report a different start.
  • GROUP carries Frame Start, so a stream holding only part of a group is self-describing. It is redundant in the steady state, and carried anyway because a SUBSCRIBE_UPDATE that moves the bound races the group streams already in flight: separate streams, no ordering between them.
  • SUBSCRIBE_OK is unchanged. It needs no frame field, because the start frame follows from Group plus the subscriber's own request. Worth stating explicitly since it is easy to get backwards: a subscriber that requested group 5 frame 15 and receives Group = 6 starts at frame 0 of group 6, not frame 15.

Model

  • group::Producer::start_at starts a group at a later frame, reusing the eviction tombstone: a reader below the offset gets Lagged either way. A group can be short at its front or its back, never in the middle, so this and finish are the only two ways to trim one.
  • group::Consumer gains index / start_at / end_at, mirroring how track::Subscriber bounds group sequences. start_at clamps up to the first frame the group still holds, which is what lets both the publisher and the spliced reader detect a missing head instead of serving one group's frames under another's numbering.
  • Subscription gains frame_start / frame_end. The aggregate folds whole (group, frame) positions; folding the group and the frame independently would invent a bound nobody asked for.
  • group::Fetch gains frame_start only. A fetch always runs to the end of the group, and a caller wanting less caps the returned consumer. Stopping the fetch short would put a group in the cache indistinguishable from a complete one, so a later fetch of the whole group would resolve from it and come up short. The wire keeps Frame End, applied by the serving publisher as a read cap, so a downstream peer still saves the transfer.
  • The fetch cache is coverage-aware. A group cached from a frame-bounded subscription starts partway in, so answering a whole-group fetch from it would silently hand back a tail; it is a miss instead, and a too-narrow entry is replaced rather than colliding on its sequence. fetch_group also returns a consumer positioned where the caller asked rather than at the group's own start.
  • resume segments are bounded by position rather than sequence, and a group straddling a boundary is itself spliced. The reader keeps one handle and pulls each route's copy in turn rather than being fed, so a reader that stops polling the subscription (exactly the moq-json case) still gets its continuation. A copy that dies, or that cannot cover the seam, stalls the group instead of erroring it, matching how a dead segment stalls the track.
  • A group tracks its committed frame count alongside its written one. create_frame advances the written count when a chunked frame is opened, so a route dying midway through one used to hand the replacement a boundary past a frame nobody ever received in full, permanently stalling any whole-frame reader. It now resumes at that frame.

Public API changes

All additive; nothing renamed, removed, or signature-changed.

  • moq_net::track::Subscription: new pub frame_start: u64, pub frame_end: Option<u64>, with_frame_start, with_frame_end. #[non_exhaustive], so external struct literals were never possible.
  • moq_net::group::Fetch: new pub frame_start: u64, with_frame_start. Same reasoning.
  • moq_net::group::Producer::start_at; moq_net::group::Consumer::{index, start_at, end_at}; moq_net::track::GroupRequest::frame_start.

Not public despite appearances:

  • The lite::{Subscribe, SubscribeUpdate, SubscribeStart, Fetch, Group} field additions are internal: mod lite is private in moq-net/src/lib.rs and only re-exports Role.
  • group::Consumer was split into Plain / Spliced variants behind the existing surface. All six public read methods were relocated; each signature is byte-identical to main.
  • The js/net lite/ changes are internal: js/net/src/index.ts does not export lite, and package.json exports only . and ./zod.

Test plan

  • nix develop --command just check — clean (rust + js + drafts + shell/md/toml/gh workflow lints).
  • nix develop --command just ci — clean; 2439 tests run, 2439 passed.
  • RUSTDOCFLAGS="-D warnings" cargo doc — clean.
  • just drafts check — parses.
  • just test smoke — passes.

New coverage: mid-group takeover splicing frames seamlessly; the old route racing past its frame cap being filtered; rolling past a finished group; a dead copy stalling until its continuation; a copy missing the head stalling rather than serving a tail as a head; a chunked frame that never completed being redelivered; position_group skipping a missing head and capping the end group; start_at / end_at / clamping on the group model; position-folding in the subscription aggregate; and wire roundtrips including a lite-05 byte-compatibility check.

Load-bearing assertions were mutation-checked rather than assumed, since a rebase onto #2526's cache rewrite could easily have made one vacuous. Each of these reverts a fix and fails:

Mutation Result
resume_position uses the written frame count 1 test fails
fetch cache coverage check removed 2 tests fail
claim_sequence coverage check removed 1 test fails
js publisher ignores startFrame 2 tests fail
js GROUP frameStart hardcoded to 0 2 tests fail
js publisher ignores endFrame 1 test fails

One that does not fail is documented under the coalescing note below.

Review follow-ups

An adversarial review pass raised three things. Two are fixed here; the third I traced and disagree with as stated.

Fixed: range-unaware fetch cache. Reachable, and introduced by this PR's resume path. A relay that subscribes upstream with a frame offset caches a partial group; a later whole-group fetch matched it on sequence alone and resolved to the tail. Now a cached group that starts above the request is a miss, a too-narrow entry is replaced rather than colliding on its sequence, and the end dimension is gone by construction (see group::Fetch above). Regression tests: fetch_ignores_a_group_that_starts_too_late, fetch_widens_only_while_queued.

Fixed: js/net publisher ignored frame bounds. Both the FETCH and the SUBSCRIBE path served the whole group, so a peer asking for a tail would have frame 0's payload land under index N. The publisher now threads the bounds from SUBSCRIBE (and any SUBSCRIBE_UPDATE) through to each group, filters on the frame's real index via readFrameSequence, and puts the actual start in the GROUP header. A group that ends before the requested start resets rather than FINning, since FINning would claim an empty group at that index when the truth is this publisher cannot serve the range.

On severity: the review called the SUBSCRIBE half corrupting, and for a Rust peer it wasn't — JS labelled its frames from 0 and a Rust splice read the ones it needed at their true indices, so it was wasted transfer rather than wrong data. Fixed regardless, because it violated the partial-group rule above and was a trap for any peer that trusted the bound.

Note on the coalescing guard

join hands back &mut to an entry a handler may already have snapshotted into an immutable GroupRequest, so widening a range after hand-off cannot reach the wire. This PR guards the widen behind "still queued". Mutation testing says that guard changes no behavior — restoring the unconditional widen still passes everything, because the write was always a no-op that merely read like a promise. What actually protects the late caller is the coverage check: it refuses a group that starts above what it asked for, so the caller fails cleanly and its retry queues a fresh attempt. The test is named for what it pins rather than for the guard.

Remaining gap

The js/net subscriber ignores GROUP.Frame Start. Its publisher now honors frame bounds on both subscriptions and fetches, but its group model has no frame-offset concept (no equivalent of group::Producer::start_at), so a received partial group is numbered from 0. A browser therefore cannot yet initiate a frame-precise resume. Not reachable today: a JS subscriber never requests a frame offset, and under the partial-group rule a publisher only sends one to a subscriber that asked.

Review round 2 (CodeRabbit)

  • JS served groups outside the requested group range. frameRange handed back a whole-group range for any sequence, and the publisher never had group bounds to begin with, so SUBSCRIBE_START could name a group below the requested start. It now returns nothing outside [startGroup, endGroup] and those groups are skipped. Regression tests on both boundaries.
  • The draft forbade a range both implementations accept. It called a Frame End "at or below Frame Start" a protocol violation, but Rust (end < start_frame) and JS (endFrame - 1 < startFrame) both allow equal bounds, which is a legal single-frame range. The spec was wrong, not the code. (The review's stated rationale — that the draft says "through the start of the group" — did not match the text; the contradiction below it was the real defect.)
  • budget overflow. end - index + 1 panics in debug when end == usize::MAX. Unreachable from the wire, since varints cap at 2^62-1, but Consumer::end_at is public. Now saturating_add.
  • Stale send_update doc and positional-parameter creep in the JS publisher (#runGroup 5 args to 2, #runTrack 6 to 3, via a ServeGroup options type) both applied.

Rebase note

Rebased twice: onto #2526 and then onto #2473.

#2526

Rebased onto main, which includes #2526 (global LRU pool replaced by per-track write-time eviction). That rewrote the structures this PR builds on: groups: VecDeque<Option<(Producer, Instant)>> became lookup: HashMap<u64, Slot>, replace_evicted was deleted in favour of claim_sequence, and Drop moved into an Alive refcount. Resolved by taking main's structure and re-deriving these semantics on top of it, most notably folding the coverage rule into claim_sequence rather than keeping a parallel function. The mutation table above is the evidence that survived the move.

#2473

The lighter of the two: one textual conflict (the lite-06 changelog, additive on both sides) plus one semantic conflict git could not see. #2473 added normative prose saying a relay may splice redundant routes "at a Group boundary", which this PR supersedes; left alone the draft would contradict its own Positions section. Updated to say the splice point is a Position, and dropped the phrase from #2473's changelog bullet so the two entries agree.

That rebase also surfaced a real defect. #2473 added two standby tests that produce a group without writing to it, and they failed: resume_position returned (G, 0) for a group created but never written, so a replacement pointed at frame 0 re-served a sequence the reader already held. The boundary now rolls to the next group unless the copy actually wrote a frame (committed_frames() > first_frame()), which is also why this PR no longer touches origin.rs at all: an earlier round had me adding finish() to tests I do not own to accommodate the old behavior, and those edits are gone.

Cross-Package Sync

drafts/, js/net, and doc/concept are updated. rs/moq-ffi is deliberately untouched: exposing the frame bounds there would ripple through four language wrappers for a primitive whose only consumer today is moq-net's own route resumption, which is transparent to FFI callers.

(Written by Opus 5)

@sourcery-ai sourcery-ai 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.

Sorry @kixelated, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@kixelated, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 2 seconds

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d6823278-bbae-4c9a-8dfc-88291611209e

📥 Commits

Reviewing files that changed from the base of the PR and between 42094bc and 8b848ed.

📒 Files selected for processing (20)
  • doc/concept/layer/moq-lite.md
  • drafts/draft-lcurley-moq-lite.md
  • js/net/src/lite/connection.ts
  • js/net/src/lite/fetch.ts
  • js/net/src/lite/group.ts
  • js/net/src/lite/publisher.test.ts
  • js/net/src/lite/publisher.ts
  • js/net/src/lite/subscribe.ts
  • js/net/src/lite/version.ts
  • rs/moq-net/src/lite/fetch.rs
  • rs/moq-net/src/lite/group.rs
  • rs/moq-net/src/lite/publisher.rs
  • rs/moq-net/src/lite/subscribe.rs
  • rs/moq-net/src/lite/subscriber.rs
  • rs/moq-net/src/lite/version.rs
  • rs/moq-net/src/model/group.rs
  • rs/moq-net/src/model/requests.rs
  • rs/moq-net/src/model/resume.rs
  • rs/moq-net/src/model/subscription.rs
  • rs/moq-net/src/model/track.rs

Walkthrough

The change adds frame-precise subscription and fetch bounds for Lite-06 and later. Wire codecs, group state, consumers, publishers, subscribers, caching, and route splicing now carry (group, frame) positions, including partial GROUP streams and bounded FETCH behavior. Older protocol versions retain group-only encoding with validation for unsupported frame bounds. Documentation and tests describe and verify the new semantics.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: frame-specific start and end bounds for subscriptions and fetches in net.
Description check ✅ Passed The description is clearly related to the changeset and explains the new frame-precise bounds, wire format updates, and model changes.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch claude/frame-specific-subs-fetches-6d6a0b

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
kixelated force-pushed the claude/frame-specific-subs-fetches-6d6a0b branch 3 times, most recently from d27ff17 to 9458d7f Compare July 28, 2026 04:25

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (2)
js/net/src/lite/publisher.ts (1)

402-409: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Bundle the added serving parameters into options objects.

The touched private methods now take five or six positional parameters. Group timescale and frame-bound values should be named fields to prevent argument-order mistakes.

As per coding guidelines, replace functions with four or more arguments or repeated groups of values with appropriate structs or abstractions.

Also applies to: 550-556, 580-580

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@js/net/src/lite/publisher.ts` around lines 402 - 409, Update the touched
private methods, including `#runTrack` and the methods at the referenced
locations, to replace four-or-more positional arguments with an options object.
Bundle the related serving parameters, including timescale and frame-bound
values, as clearly named fields and update every call site to pass and consume
the object while preserving behavior.

Source: Coding guidelines

rs/moq-net/src/model/requests.rs (1)

108-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Match the surrounding style: use the already-imported Borrow/Hash and the same trait-bound ordering as the sibling methods.

Every other method here writes K: Borrow<Q>, Q: Eq + Hash + ?Sized. Visibility also differs (pub(crate) vs pub); intentional narrowing is fine, just flagging the inconsistency.

♻️ Proposed tidy-up
-	pub(crate) fn is_queued<Q>(&self, key: &Q) -> bool
-	where
-		K: std::borrow::Borrow<Q>,
-		Q: std::hash::Hash + Eq + ?Sized,
-	{
+	pub(crate) fn is_queued<Q>(&self, key: &Q) -> bool
+	where
+		K: Borrow<Q>,
+		Q: Eq + Hash + ?Sized,
+	{
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rs/moq-net/src/model/requests.rs` around lines 108 - 113, Update is_queued’s
trait bounds to use the already-imported Borrow and Hash types, matching the
sibling method ordering of K: Borrow<Q> and Q: Eq + Hash + ?Sized. Preserve its
existing pub(crate) visibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@drafts/draft-lcurley-moq-lite.md`:
- Around line 1141-1149: Update the FETCH “Frame End” documentation to preserve
0 as the unbounded-through-end default and describe non-default values using
decoded inclusive semantics: the encoded value is the absolute inclusive frame
index plus 1. Keep the existing protocol-violation rule for decoded values at or
below “Frame Start.”

In `@js/net/src/lite/publisher.ts`:
- Around line 73-77: Update frameRange to return no range for sequences before
startGroup or after endGroup, while preserving the existing startFrame/endFrame
behavior within the requested interval. In `#runTrack`, skip consumers whose
frameRange is absent, and ensure runSubscribe passes the bounded ranges to
broadcast.subscribe so SUBSCRIBE_START and SUBSCRIBE_END reflect the request.
Add a regression test covering groups on both sides of each boundary.

In `@rs/moq-net/src/lite/subscriber.rs`:
- Around line 1396-1413: Update the documentation comment for `send_update` to
state that it varies both `end_group` and `end_frame`, matching the parameters
forwarded into `lite::SubscribeUpdate`; leave the implementation unchanged.

In `@rs/moq-net/src/model/group.rs`:
- Around line 977-979: Update the budget calculation in the consumer buffering
logic around self.end and index to avoid overflowing when the inclusive end
equals usize::MAX. Preserve the existing usize::MAX/unbounded behavior and
ensure the computed budget remains valid for end == usize::MAX, including index
== 0, so buffering does not stall.

---

Nitpick comments:
In `@js/net/src/lite/publisher.ts`:
- Around line 402-409: Update the touched private methods, including `#runTrack`
and the methods at the referenced locations, to replace four-or-more positional
arguments with an options object. Bundle the related serving parameters,
including timescale and frame-bound values, as clearly named fields and update
every call site to pass and consume the object while preserving behavior.

In `@rs/moq-net/src/model/requests.rs`:
- Around line 108-113: Update is_queued’s trait bounds to use the
already-imported Borrow and Hash types, matching the sibling method ordering of
K: Borrow<Q> and Q: Eq + Hash + ?Sized. Preserve its existing pub(crate)
visibility.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 908cbad6-94e6-47b8-8ae4-a1207e8a7664

📥 Commits

Reviewing files that changed from the base of the PR and between 67720fa and 9458d7f.

📒 Files selected for processing (21)
  • doc/concept/layer/moq-lite.md
  • drafts/draft-lcurley-moq-lite.md
  • js/net/src/lite/connection.ts
  • js/net/src/lite/fetch.ts
  • js/net/src/lite/group.ts
  • js/net/src/lite/publisher.test.ts
  • js/net/src/lite/publisher.ts
  • js/net/src/lite/subscribe.ts
  • js/net/src/lite/version.ts
  • rs/moq-net/src/lite/fetch.rs
  • rs/moq-net/src/lite/group.rs
  • rs/moq-net/src/lite/publisher.rs
  • rs/moq-net/src/lite/subscribe.rs
  • rs/moq-net/src/lite/subscriber.rs
  • rs/moq-net/src/lite/version.rs
  • rs/moq-net/src/model/group.rs
  • rs/moq-net/src/model/origin.rs
  • rs/moq-net/src/model/requests.rs
  • rs/moq-net/src/model/resume.rs
  • rs/moq-net/src/model/subscription.rs
  • rs/moq-net/src/model/track.rs

Comment thread drafts/draft-lcurley-moq-lite.md
Comment thread js/net/src/lite/publisher.ts Outdated
Comment thread rs/moq-net/src/lite/subscriber.rs Outdated
Comment thread rs/moq-net/src/model/group.rs Outdated
@kixelated
kixelated force-pushed the claude/frame-specific-subs-fetches-6d6a0b branch from 9458d7f to 36ad19d Compare July 28, 2026 05:28

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@js/net/src/lite/publisher.test.ts`:
- Around line 144-149: Extend the bounded subscription tests around serveBounded
with a case spanning multiple groups, using distinct start and end groups.
Assert that startFrame limits only the start group, inclusive endFrame limits
only the end group, and intermediate groups are fully served; preserve the
existing single-group test.
- Around line 193-210: Update the incoming stream read loop around reader.read()
to retain the pending read and its timeout handle, clear the timer when either
settles, and on timeout await reader.cancel() before releasing the reader lock.
Preserve normal stream processing and ensure cleanup handles the pending read
without an unhandled rejection.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ee9445e8-17b6-4707-96a9-21c2304b7a33

📥 Commits

Reviewing files that changed from the base of the PR and between 9458d7f and 36ad19d.

📒 Files selected for processing (20)
  • doc/concept/layer/moq-lite.md
  • drafts/draft-lcurley-moq-lite.md
  • js/net/src/lite/connection.ts
  • js/net/src/lite/fetch.ts
  • js/net/src/lite/group.ts
  • js/net/src/lite/publisher.test.ts
  • js/net/src/lite/publisher.ts
  • js/net/src/lite/subscribe.ts
  • js/net/src/lite/version.ts
  • rs/moq-net/src/lite/fetch.rs
  • rs/moq-net/src/lite/group.rs
  • rs/moq-net/src/lite/publisher.rs
  • rs/moq-net/src/lite/subscribe.rs
  • rs/moq-net/src/lite/subscriber.rs
  • rs/moq-net/src/lite/version.rs
  • rs/moq-net/src/model/group.rs
  • rs/moq-net/src/model/requests.rs
  • rs/moq-net/src/model/resume.rs
  • rs/moq-net/src/model/subscription.rs
  • rs/moq-net/src/model/track.rs
🚧 Files skipped from review as they are similar to previous changes (18)
  • rs/moq-net/src/model/requests.rs
  • rs/moq-net/src/lite/version.rs
  • rs/moq-net/src/lite/group.rs
  • rs/moq-net/src/lite/fetch.rs
  • js/net/src/lite/connection.ts
  • js/net/src/lite/version.ts
  • doc/concept/layer/moq-lite.md
  • rs/moq-net/src/lite/subscribe.rs
  • js/net/src/lite/group.ts
  • js/net/src/lite/subscribe.ts
  • js/net/src/lite/publisher.ts
  • js/net/src/lite/fetch.ts
  • rs/moq-net/src/lite/publisher.rs
  • rs/moq-net/src/model/subscription.rs
  • rs/moq-net/src/lite/subscriber.rs
  • rs/moq-net/src/model/track.rs
  • rs/moq-net/src/model/group.rs
  • rs/moq-net/src/model/resume.rs

Comment thread js/net/src/lite/publisher.test.ts Outdated
Comment thread js/net/src/lite/publisher.test.ts Outdated
@kixelated
kixelated force-pushed the claude/frame-specific-subs-fetches-6d6a0b branch from 36ad19d to 42094bc Compare July 28, 2026 05:53

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
rs/moq-net/src/model/requests.rs (1)

104-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a direct test for is_queued's queued→popped transition.

is_queued gates whether a coalesced fetch's frame_start may still be widened (see track.rs::fetch_group), so it directly protects frame-precise coverage correctness. The existing tests exercise pop/join/drain_queued but never assert on is_queued itself.

✅ Proposed test
 	fn popped_stays_joinable_until_removed() {
 		let mut requests = Requests::<u64, &str>::default();
 		requests.add_handler();
 		assert!(requests.insert(1, "a").is_ok());
+		assert!(requests.is_queued(&1));
 
 		assert_eq!(requests.pop(), Some(1));
 		assert_eq!(requests.join(&1), Some(&mut "a"));
 		assert!(!requests.has_queued());
+		assert!(!requests.is_queued(&1));
 
 		assert_eq!(requests.remove(&1), Some("a"));
 		assert!(requests.is_empty());
 	}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rs/moq-net/src/model/requests.rs` around lines 104 - 115, Add a focused test
for Requests::is_queued that inserts or queues a request, asserts it returns
true, pops the request, then asserts it returns false while preserving the
existing ability to join the popped request. Place the test alongside the
existing pop/join/drain_queued tests and exercise the queued-to-popped
transition directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@rs/moq-net/src/model/requests.rs`:
- Around line 104-115: Add a focused test for Requests::is_queued that inserts
or queues a request, asserts it returns true, pops the request, then asserts it
returns false while preserving the existing ability to join the popped request.
Place the test alongside the existing pop/join/drain_queued tests and exercise
the queued-to-popped transition directly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ce52cef5-a393-49f3-85e4-b52049e4bec6

📥 Commits

Reviewing files that changed from the base of the PR and between 36ad19d and 42094bc.

📒 Files selected for processing (20)
  • doc/concept/layer/moq-lite.md
  • drafts/draft-lcurley-moq-lite.md
  • js/net/src/lite/connection.ts
  • js/net/src/lite/fetch.ts
  • js/net/src/lite/group.ts
  • js/net/src/lite/publisher.test.ts
  • js/net/src/lite/publisher.ts
  • js/net/src/lite/subscribe.ts
  • js/net/src/lite/version.ts
  • rs/moq-net/src/lite/fetch.rs
  • rs/moq-net/src/lite/group.rs
  • rs/moq-net/src/lite/publisher.rs
  • rs/moq-net/src/lite/subscribe.rs
  • rs/moq-net/src/lite/subscriber.rs
  • rs/moq-net/src/lite/version.rs
  • rs/moq-net/src/model/group.rs
  • rs/moq-net/src/model/requests.rs
  • rs/moq-net/src/model/resume.rs
  • rs/moq-net/src/model/subscription.rs
  • rs/moq-net/src/model/track.rs
🚧 Files skipped from review as they are similar to previous changes (17)
  • rs/moq-net/src/lite/fetch.rs
  • js/net/src/lite/fetch.ts
  • rs/moq-net/src/lite/version.rs
  • js/net/src/lite/connection.ts
  • rs/moq-net/src/lite/group.rs
  • js/net/src/lite/group.ts
  • js/net/src/lite/subscribe.ts
  • js/net/src/lite/version.ts
  • rs/moq-net/src/lite/subscriber.rs
  • doc/concept/layer/moq-lite.md
  • js/net/src/lite/publisher.ts
  • rs/moq-net/src/model/subscription.rs
  • rs/moq-net/src/lite/publisher.rs
  • rs/moq-net/src/lite/subscribe.rs
  • rs/moq-net/src/model/resume.rs
  • rs/moq-net/src/model/track.rs
  • rs/moq-net/src/model/group.rs

Route migration splices per-session tracks at group boundaries, which never
resumes a track whose current group can stay open indefinitely: a moq-json
append log keeps its whole history in one group, and a quiet catalog may not
roll for a long time. A route dying partway through such a group stalled the
logical subscriber until a group that might never come.

Bounds are now frame-precise, end to end.

A partial group only ever goes to a subscriber that asked for one. A publisher
that cannot serve a group from the frame the subscription names skips it and
resolves to a later group, rather than delivering the part it holds. A group is
the unit of decodability, so dropping leading groups leaves a stream the
application can still decode while dropping leading frames often does not, and
only the subscriber knows whether a partial group is any use to it. That keeps
every subscriber which does not opt in on exactly the pre-lite-06 behavior.

moq-lite-06 (draft + implementation):
- SUBSCRIBE / SUBSCRIBE_UPDATE carry `Frame Start` and `Frame End`, qualifying
  the start and end group. `Frame Start` is a plain index; `Frame End` is the
  index + 1, matching `Group End`, so two abutting subscriptions carry the same
  numbers on the wire.
- FETCH carries the same pair, bounding the returned frames within the group. A
  publisher that cannot serve the range in full resets, since the response has
  no header to report a different start.
- GROUP carries `Frame Start`, so a stream holding only part of a group is
  self-describing. Redundant in the steady state, and carried anyway because a
  SUBSCRIBE_UPDATE that moves the bound races the group streams already in
  flight.
- SUBSCRIBE_OK is unchanged. It needs no frame field: the start frame follows
  from `Group` and the subscriber's own request, and a subscriber that asked for
  group 5 frame 15 and receives group 6 starts at frame 0.

Model:
- `group::Producer::start_at` starts a group at a later frame, reusing the
  eviction tombstone: a reader below the offset gets `Lagged` either way. A
  group can be short at its front or its back, never in the middle, so this and
  `finish` are the only two ways to trim one.
- `group::Consumer` gains `index` / `start_at` / `end_at`, mirroring how
  `track::Subscriber` bounds group sequences. `start_at` clamps up to the first
  frame the group still holds, which is what lets both the publisher and the
  spliced reader detect a missing head instead of serving one group's frames
  under another's numbering.
- `Subscription` gains `frame_start` / `frame_end`. The aggregate folds whole
  (group, frame) positions, since folding the two independently would invent a
  bound nobody asked for.
- `group::Fetch` gains `frame_start` only. A fetch always runs to the end of the
  group and a caller wanting less caps the returned consumer, because a fetch
  that stopped short would cache a group indistinguishable from a complete one.
  The wire keeps `Frame End`, applied by the serving publisher as a read cap.
- The fetch cache is coverage-aware: a group cached from a frame-bounded
  subscription starts partway in, so answering a whole-group fetch from it would
  hand back a tail. It is a miss instead, and a too-narrow entry is replaced
  rather than colliding on its sequence.
- `resume` segments are bounded by position rather than sequence, and a group
  that straddles a boundary is itself spliced: the reader keeps one handle and
  pulls each route's copy in turn. A copy that dies, or that cannot cover the
  seam, stalls the group instead of erroring it, matching how a dead segment
  stalls the track.
- A group tracks its committed frame count alongside its written one. A route
  dying midway through a chunked frame resumes *at* that frame rather than after
  it: only the dead route saw the payload, and only part of it, so a reader can
  do nothing with it but receive it again whole.

The `js/net` publisher honors the same bounds when serving a subscription or a fetch,
so a peer that asks for part of a group gets what it asked for rather than the whole
thing renumbered. Its subscriber still ignores `GROUP.Frame Start`, which needs a
frame-offset concept in the JS group model; a browser therefore cannot yet initiate a
frame-precise resume, but nothing it does today misreads one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kixelated
kixelated force-pushed the claude/frame-specific-subs-fetches-6d6a0b branch from 42094bc to 8b848ed Compare July 28, 2026 06:15
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.

net: splice route and connection changes within an active group

1 participant