Repository navigation
fix(net): backport an explicit group 0 floor on pre-06 wires (#5000) - #5300
Conversation
fix(net): deliver a fresh group below the first served group when the floor allows it (#5000) Backported to release: ported the new late_lower_group test to release's tokio harness and max_age naming, kept release's SubscribeStart (no largest field), dropped the quiet_catalog hunk (deleted on release) and the lite-live quest (absent on release), and added a wire-value test that pins Rust and JS to the same pre-06 Group Start encoding. (cherry picked from commit 36f82ac) Co-authored-by: Grok 4.7 <noreply@x.ai> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acdcf4a2fb
ℹ️ 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".
| if self.track.subscription().start.is_none() && !self.ctx.version.resolves_start() { | ||
| self.track.raise_start_to(start); |
There was a problem hiding this comment.
When a pre-06 subscription starts with an explicit group 0, serves its first group, and then receives a SUBSCRIBE_UPDATE with Group Start omitted, this condition has already evaluated false and update() only calls start_at for Some. The cursor therefore remains at 0 and transmits later lower groups even though wire value 0 means “latest”; re-pin it to the current live edge when an update clears the explicit floor, including in the matching js/net/src/lite/publisher.ts update path. (Written by GPT-5.6 Sol)
AGENTS.md reference: rs/moq-net/AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pre-06 drafts define an absent Group Start as the latest group, and position_cursor applies that on SUBSCRIBE. A SUBSCRIBE_UPDATE that drops a named floor left the cursor at the old floor, so a late group below the live edge was still served. Re-pin to the latest group in Rust and JS. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit d60481d)
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: e72ccaa.
Direction: sound, narrowly scoped backport. The matching Rust/JS codec and publisher changes preserve explicit group-zero floors without changing aggregation. The added update handling addresses the independent re-pin finding when a latest group exists.
The empty-track residual reported on #5301 also remains here: Rust lines 2345–2351 and JS lines 795–803 preserve the old nonzero cursor when latest is absent. Clear that floor and add the empty-track case alongside the new populated-track tests. This is incomplete repair of a pre-existing defect, not a new backport regression.
Verification: full 15-file diff, surrounding cursor/update logic, tests, and prior discussion inspected through GitHub. No tests executed; head-specific CI success has not been established. PR remains open and non-draft.
(Written by OpenAI)
# Conflicts: # rs/moq-net/src/lite/publisher.rs
With no latest group, an update that drops a named floor kept the old floor, so a first group below it was skipped. Pin to 0 instead: on an empty track the next group is the latest. Found by CodeRabbit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit fcc4830)
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 80ea757.
The previously reported empty-track residual is fixed: Rust and JS now reset to group 0 when a pre-06 update drops its floor before any group exists. Both languages add the requested nonzero-floor/empty-track regression.
No new actionable findings in the substantive update since e72ccaa, accounting for the release merge. Direction remains sound and minimal; the Rust conflict resolution preserves release’s lite-06+ quiet-catalog floor clearing alongside the pre-06 re-pin.
Verification: GitHub-only inspection of the delta, merge context, cursor handling and tests; no tests executed. Head-specific Check/Test and platform CI remain queued/running.
(Written by OpenAI)
|
Summary before merge:
(Written by Claude Opus 5.5) |
1 participant
Backport of #5000 (36f82ac) onto
release.What it fixes
On
release, a moq-lite-05 (or older) SUBSCRIBE or SUBSCRIBE_UPDATE that names group 0 was folded to "latest" by the encoder in both languages, so a subscriber asking to replay from group 0 joined at the live edge instead. After this:undefined/Nonemeans the latest group (wire 0 on pre-06).Subscription::start: Option<Position>and codecstart_group: Option<u64>encode exactly like JSgroups.startundefined vs{ included: 0 }.The publisher no longer pins its cursor to the SUBSCRIBE_START group when the subscription named an explicit floor, so a fresh group created below the first served group is still delivered within the subscriber's max age. A pre-06 subscription with no floor still pins, since those drafts define an absent Group Start as the latest group.
Conflicts and resolutions
rs/moq-net/src/lite/subscribe.rs: kept release'sBufMutcodec (no refactor(net)!: plain u64 varint codec and simulated-time tests (rs2ts) #4437 varint refactor); the fix isstart_group.encode(w, version)with no> 0filter. Test ported to release'sencode_msg/decode_msg.rs/moq-net/src/lite/publisher.rs: kept release'sSubscribeStart { group }(nolargest); applied the conditionalraise_start_to. Unit tests use#[tokio::test]andwith_max_age.rs/moq-net/tests/late_lower_group.rs(new): ported to release's tokio harness,request_broadcast(path),track.used(), andmax_age.rs/moq-net/tests/history_groups.rs: kept release's version (main's hunk only rewrote a comment on a sleep release does not have).rs/moq-net/tests/quiet_catalog.rs: deleted on release, hunk dropped.js/net/src/integration.test.ts,js/net/src/lite/subscriber.ts:maxAgeinstead ofmaxDelay; merged theresolvesStartimport with release's.drafts/draft-lcurley-moq-lite.md: kept the SUBSCRIBE / SUBSCRIBE_OK text and the two matching lite-07 changelog lines; dropped main-only changelog entries. No wire format change.quest/m1: deletedlite-late-lower-group.mdand its README line, as main did; dropped thelite-live.mdedit (that quest is absent on release).Added
group_start_wire_values_match_js(Rust) and a matching JS test that pin the pre-06 wire values (absent 0, group 0 is 1) and lite-06 (both 0) on both sides.Public API and wire impact
No public API change. Wire: a pre-06 SUBSCRIBE/SUBSCRIBE_UPDATE that names group 0 now sends
Group Start = 1instead of 0, as those drafts define. Lite-06 and lite-07-wip bytes are unchanged. SUBSCRIBE_OKGroupis no longer a new floor (a semantic change documented in the lite-07 draft).Tests
just check: pass.cargo nextest run -p moq-net: 1545 passed, 4 skipped. With the release encoder and publisher restored,late_lower_groupfails on lite-05/06/07-wip andgroup_start_wire_values_match_jsfails.bun testinjs/net: 1119 pass, 2 skip. With the release encoder restored, the three Group Start tests fail.Also: #5301
Codex found that a pre-06 SUBSCRIBE_UPDATE dropping a named floor left the cursor at the old floor (also on
main). It is fixed onmainin #5301, and cherry-picked here. Release'supdate()has no lowered-floor demand accounting, so only the re-pin branch applies, folded into thematchthat release's quiet-catalog backport (#5228) added afterreleasewas merged in. It includes #5301's follow-up for an empty track (pin to 0 when there is no latest group, found by CodeRabbit). Both Rust tests share alite05_runhelper, since release has no genericlite_run. The Rust test is ported to release's harness (#[tokio::test],max_age).just checkpasses, and the test fails without the fix.Decision (maintainer): fix on
mainfirst, then include it in this backport rather than merge without it.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code