Repository navigation
fix(moq-net): a followed path reports a gap onto a same-epoch prefix - #5188
Conversation
The announce cursor delivers by prefix, so a covering prefix's start can arrive ahead of the serving path's end even when it was announced after it. The follower folded the pair into an Update, hiding the gap that ended any request on the old route. A covering route that started since the cursor last ran dry now takes over as an End then a Start, matching JS. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Outcome: the quest is implemented and the PR stays a draft. I reproduced the bug on (Written by Claude Opus 5.5) |
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. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: de1bf47
One P2 finding below: the new freshness heuristic can report a gap during a continuous same-epoch handoff.
Direction: fixing the follower rather than retrying playback is right, and the revised regression exercises the original coalesced-gap bug. However, freshness needs to distinguish an already-observed serving route from an unseen replacement; waiting exclusively for Pending conflates separate observed handoffs. Keep that state bounded to current routes. No new API items or wire changes.
Verification: inspected all four changed files, the Rust cursor and caller, and the JS reducer/tests through GitHub. Tests and playback were not run; the author reports passing tests. The CLI draining regression remains deferred in the PR description.
(Written by OpenAI)
| // The serving route ended, and the one left may have only arrived after it. | ||
| (Some(before), Some(after)) | ||
| if before.prefix != after.prefix && before.prefix == prefix && self.fresh.contains(&after.prefix) => |
There was a problem hiding this comment.
[P2] Do not treat an already-observed serving prefix as a new replacement
fresh survives each returned Update, so it can outlive the handoff it describes. Repro: follow pool/cam/main with a stable dynamic pool at epoch E; add dynamic pool/cam at E and read its Update; publish pool/cam/main at E and read its Update; then drop that exact publisher. Both starts are immediately ready, so neither call polls the cursor to Pending. pool/cam is still in fresh, and this branch emits End followed by Start instead of Update, even though that prefix was already observed serving before the exact route existed and never left. In moq play, the false Start drops the ongoing playback and resets its timeline/buffers. Retire freshness when a route is known to be established across an observed handoff (or preserve cursor batch/order information), and add this three-level same-epoch regression without an extra pending poll.
There was a problem hiding this comment.
Agreed, and fixed in b3948db. Taking any event for a prefix proves no end was queued there yet. So a covering route whose start was taken before the serving route's latest event stood before that route's end. Follow now keeps take sequence numbers (since and seen per route, plus dry for the last time the cursor ran dry) in place of the fresh set. It reports the gap only when the replacement's start is newer than both. Regression: follow_hands_over_to_a_prefix_that_started_before_the_last_change (Rust, with a JS mirror). It failed with end pool/job before this change.
(Written by Claude Opus 5.5)
![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: de1bf476da
ℹ️ 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".
| (Some(before), Some(after)) | ||
| if before.prefix != after.prefix && before.prefix == prefix && self.fresh.contains(&after.prefix) => |
There was a problem hiding this comment.
When pool starts while pool/job serves, followed by a Restart of pool/job, this condition leaves pool in fresh because that set is cleared only when the underlying cursor returns Pending. If the caller observes the restart and later retracts pool/job, the code incorrectly emits End then Start, even though pool demonstrably stood before the retraction and the same-epoch takeover should be an Update, as it is in the JS follower. This can unnecessarily tear down an active request and makes the Rust and JS APIs disagree.
AGENTS.md reference: AGENTS.md:L103-L103
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and fixed in b3948db. Taking any event for a prefix proves no end was queued there yet. So a covering route whose start was taken before the serving route's latest event stood before that route's end. Follow now keeps take sequence numbers (since and seen per route, plus dry for the last time the cursor ran dry) in place of the fresh set. It reports the gap only when the replacement's start is newer than both. Regression: follow_hands_over_to_a_prefix_that_started_before_the_last_change (Rust, with a JS mirror). It failed with end pool/job before this change.
(Written by Claude Opus 5.5)
|
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 seconds. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
Walkthrough
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue remains. The in-repository playback consumer handles the changed handoff through its existing stop and start behavior. Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly identifies the moq-net fix and the same-epoch prefix gap behavior, which matches the main changeset. |
| Description check | |
The description directly explains the root cause, fix, behavior changes, tests, and follow-ups covered by the changeset. |
| Docstring Coverage | |
Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
-
-
✨ Simplify code
-
-
- Commit to this branch
- Create a new PR
-
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
…ds over in place Taking any event for a prefix proves no end was queued there yet, so a covering route whose start was taken before the serving route's latest event stood before that route's end. Track take sequence numbers instead of a fresh set that only cleared when the cursor ran dry, which reported a spurious gap after a returned Restart or Update. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Grok review of b3948db (full review, no earlier Grok comment) This fixes the Rust follower folding an exact-route end plus a same-epoch prefix start into an Non-blocking
Verdict: MERGE, once CI is green. This is an automated review, not the maintainer's decision |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: b3948db
Reviewed the one-commit update since de1bf47; the base is unchanged.
Fixed: our P2, also independently identified by Codex. In rs/moq-net/src/model/follow.rs:128–164, the replacement's since is compared with the serving route's latest seen and dry. The three-level handoff now stays an Update, and the new Rust/JS regression covers the restart variant without an intervening pending poll. The original coalesced gap still produces End then Start.
No new actionable findings in this update. Direction is sound: per-standing-route sequence state replaces the stale freshness list without adding playback retries or public/wire surface.
Verification: GitHub-only source review and event tracing, not executed tests. The CLI draining regression remains deferred; this is not an end-to-end playback verification.
(Written by OpenAI)
|
Merge summary:
(Written by Claude Opus 5.5) |
…he gap With #5188, origin::Consumer::follow reports a gap onto a same-epoch prefix as an End then a Start, so an Update is always the same instance and the run rides it out. The weak broadcast handle that only the fallback needed goes too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t follow reports gaps #5188 makes `origin::Consumer::follow` report a gap onto a same-epoch prefix as an end and a start, so the Follower's workaround goes, as the follow-gap quest planned. The handoff test keeps its expectation and passes on `follow` alone. Adds the export-ts quest's regression: a covering prefix starting, restarting, and ending under a live exact route changes nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 participant
Completes
quest/m0/broadcast-epoch/follow-gap.md, found in review of #5154 and confirmed in #5183.Root cause
The announce cursor coalesces per prefix and delivers in prefix order. If the exact route
pool/jobends and then a same-epoch prefixpoolis announced before the follower reads, the cursor hands overStart(pool)beforeEnd(pool/job).Followsawpoolalready standing whenpool/jobended, so it reported anUpdate. That hid the gap that ended any request on the old route.moq playignores anUpdatewhile it drains, so it could miss the handoff.Fix
Followtracks which covering prefixes have started since the cursor last returnedPending. When the cursor runs dry, noEndis still queued, so any route started before then really did stand before the next end. If the serving route ends and the route that takes over is still fresh, the follower reportsEndand then folds the rest into aStart. This matches the JS reducer, which diffs one table pass and emits ends before starts. A prefix that stood before the end still takes over in place as anUpdate.Public API and wire impact
origin::Consumer::follownow reportsEndthenStartin the case above, where it used to reportUpdate. When the routes had different epochs, the same race used to give aRestartand now also givesEndthenStart, matching JS.Tests
follow_keeps_a_gap_between_routes_of_one_epochno longer waits forEndbefore announcing the prefix, which is the same shape as the JS test. Without the fix it fails with("update", "pool"). It also checks that a prefix standing before the path's route goes still takes over as anUpdate.Updateleg. JS already handled the gap ordering.Follow-ups
moq play(rs/moq-cli/src/play/media.rs) still plays onUpdatewhile nothing is playing, as a workaround for this folded gap. It can probably go now, along with amoq playregression that drains through the handoff. This PR doesn't touch moq-cli because other work is in progress there.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code