Repository navigation
fix(net): resolve subscription starts past skipped groups - #6028
Conversation
Co-Authored-By: GPT-6.1-Sol <noreply@openai.com>
Co-Authored-By: GPT-6.1-Sol <noreply@openai.com>
|
Validation at (written by GPT-6.1-Sol) |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Ranges given up before SUBSCRIBE_START but above the resolved start were cleared, so lite-05+ subscribers waited until the end sweep for them. Forward them as SUBSCRIBE_DROP right after the start. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 23 minutes. View limit details
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. Comment |
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. |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![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: d91db850d0
ℹ️ 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".
| for range in skipped.iter() { | ||
| self.drop_range(range.start.max(start)..range.end, writer)?; |
There was a problem hiding this comment.
When poll_forward_drop drains a drop before SUBSCRIBE_START and that sequence is re-created before the start resolves, skipped still contains the held group's sequence. This loop emits SUBSCRIBE_DROP for it, but start() immediately calls self.serve(group), so the peer accounts the group as permanently unavailable and then receives its stream, potentially settling or discarding it. This can occur when an initial group is cut and recovered before any later group resolves the start; exclude the held group from the buffered drops or do not serve it.
Useful? React with 👍 / 👎.
#6028 added a `history` argument to the bench id without a specifier, which fails to compile the bench on main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem
A warm relay could name stale cached groups in SUBSCRIBE_OK that it had already given up, while a cold relay named the actual delivery start.
Approach
Record missing-head, stale, and upstream-dropped ranges before resolving the start. Reuse the cursor's drop notifications and keep ranges compact. Skip recording a missing head at
u64::MAX, which has no successor, without rejecting the supported sequence.Once the start resolves, any recorded range above it is sent as SUBSCRIBE_DROP right after SUBSCRIBE_START, as it would have been had it been given up after the start. Without this, consuming the cursor's drop notifications before the start would hide them until the end sweep.
Mock-time tests cover cold/warm publisher caches, an actual two-hop relay on lite-05/06/07, a million-group upstream drop, a range dropped above a start resolved from the source, and the maximum missing-head sequence. The session join benchmark sweeps publisher count against retained history and runs in the existing nightly benchmark harness.
Impact
Decisions
Picked without a maintainer decision, recommended options:
poll_forward_dropguard after moq-dev/moq#5983 madeservedpresent on every lite-05+ version:!self.emit_range.served.is_none()clause, which can no longer be true there.Alternatives
Port only this independent fix from closed moq-dev/moq#5931. SUBSCRIBE_ALL and resolved upstream demand changes remain separate work.
Follow-ups
Full browser and GStreamer interop remain unverified on this host. There are no open implementation decisions.
Checks
origin/main, including moq-dev/moq#5924 and moq-dev/moq#5983; the benchmark history loop now usesShape::group_frames.[2, 0], expected[2, 2].a_range_skipped_above_the_start_is_droppedfails without the forwarding ([], expected[(2, 3)]).just checkpasses on127f92e1d(7,662 Rust tests). Two earlier runs on a host at load 40+ hit wall-clock flakes outside this change:moq-uring clock::tests::host_tracks_moq_clock(1 ms calibration slack; passes alone) andmoq-cli export::redundant_importers_switch_through_bufferbloat_on_lite_07("stitching export exited"), which also fails withmain'spublisher.rs(1/1 under load) and passes 2/3 here.0901088b7: all 27 new benchmark points pass as a debug smoke measurement (no speedup claim), and the available native interop matrix passes (publishersrust,python,go,cpp; subscribersrust,python,go,cpp,js-native-node,js-native-bun,c).just test interop --allcould not run browser or GStreamer cells on this host (pinned Chromium 1248 lackslibglib-2.0.so.0; GStreamer plugin discovery unavailable), so it is not a full-all interop pass.(Written by GPT-6.1-Sol, updated by Claude Opus 5.5)
🤖 Generated with Claude Code