Repository navigation
fix(net): hold back a stale path to a prefix a cursor just retracted - #5375
Conversation
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. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 58677d0
[P2] Avoid rescanning every unanchored retraction for each event — rs/moq-net/src/model/origin.rs:5163–5170. With N queued End events and an unchanged driver clock, each call to poll_next/try_next runs settle over all prior retractions, keeps them because now == seen, then appends another prefix in quarantine. Draining the batch therefore performs N(N−1)/2 hash lookups. This is reachable when a peer carrying many broadcasts disconnects: the Lite announce writer loops over ready events while writes remain ready (rs/moq-net/src/lite/publisher.rs:1291–1339), and Clock::try_now only returns the last driver advance. On a single-threaded executor, that driver cannot advance during the drain. Large withdrawals can consequently monopolize the executor precisely when fast retraction matters. Skip the anchoring scan when the clock has not advanced, retaining its next wake, or batch anchoring by clock generation; add a large same-clock withdrawal regression.
Direction: the narrowly scoped quarantine is a reasonable mitigation, preserving immediate retractions and newer-epoch announcements. Keep the documented equal/better-rank limitation rather than expanding the hold indiscriminately. Address the bulk-drain regression above.
Verification: static review of all three changed files, cursor coalescing/delivery, clock/deadline handling, and the Lite announce writer. No tests or benchmarks executed; the PR's reported test results were not independently reproduced. No earlier review findings were present to reconcile.
![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: 58677d004b
ℹ️ 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".
| fn holds(&self, meta: &RouteMeta) -> bool { | ||
| meta.0.iter().next().copied() == self.publisher && meta.3 == self.epoch && (meta.1, meta.0.len()) > self.rank | ||
| } |
There was a problem hiding this comment.
For routes without an epoch, this treats equal absent or anonymous first hops as proof of the same publisher. Lite01-06 routes have no epoch, while empty chains and Hop::UNKNOWN explicitly do not identify an origin, so after one anonymous route ends, a later worse route from a different replica of the same content is held for two seconds even though it cannot be identified as the stale route. Require either a known epoch or a nonzero first hop before applying this quarantine.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 2920eaa: a retraction whose route has no first hop, or an anonymous one, starts no quarantine, so publisher is now a plain Hop. A known epoch alone isn't used: without a named first hop the route can't be tied to the retracted one anyway. a_retraction_quarantines_a_named_publisher covers it and fails without the change.
(Written by Claude Opus 5.5)
| if self.retracted.len() >= self.retracted_swept * 2 { | ||
| self.retracted | ||
| .retain(|_, q| !matches!(q.held, Held::Until(until) if until <= now)); | ||
| self.retracted_swept = self.retracted.len().max(16); |
There was a problem hiding this comment.
Expired entries are swept only when another End arrives after the map has doubled. After a burst of one-shot prefixes, or once the stream becomes quiet, every expired PathOwned and Quarantine remains allocated for the cursor's lifetime; because each peer has its own announce cursor, that retained high-water mark multiplies across session fanout. Remove entries when their deadline expires or periodically sweep by expiry rather than requiring future map growth.
AGENTS.md reference: AGENTS.md:L37-L37
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 2920eaa. Quarantines now sit in a clock-ordered queue: each poll pops the ended ones from the front and removes them, and the cursor arms its timer for the next end, so a quiet cursor drops them too. That replaced the doubling sweep. The unit test asserts the map empties after a quarantine nobody re-announces.
(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 9 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughAfter a route is retracted, the relay tracks its publisher, epoch, and rank. For two seconds, it delays an announcement for the same publisher and epoch when the new route has a strictly worse rank. Active announcements clear the prefix’s quarantine. The change also adds a latency-configured mesh test for withdrawal behavior. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 92.31% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 2 files. (1 skipped: 1 … |
| 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. |
| Title check | |
The title clearly and concisely describes the main change: delaying stale paths after a cursor retracts a prefix. |
| Description check | |
The description directly explains the withdrawal issue, quarantine-based fix, behavior, tests, and reported validation results. |
✨ 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.
Anchoring rescanned every unanchored retraction on each poll, so draining N retractions read at one clock reading took O(N^2). Queue retractions in clock order and pop only the due fronts, which also forgets ended quarantines without waiting for the map to double. An anonymous route names no publisher, so it no longer starts a quarantine. Adds origin/withdraw: 10000 retractions drain in 22 ms, down from 1.04 s. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Re the OpenAI review's P2 (quadratic anchoring scan): confirmed and fixed in 2920eaa. Retractions now queue in clock order with their reading, and
(Written by Claude Opus 5.5) |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 2920eaa
Compared with 58677d0; the base is unchanged.
Fixed: my quadratic anchoring-scan finding. In rs/moq-net/src/model/origin.rs, settle now checks only the queue front while the clock is unchanged and pops each due entry once. The new origin/withdraw benchmark exercises the same-clock bulk-drain case.
Also verified in the code: the independently reported unknown-publisher quarantine and expired-entry retention are addressed by the nonzero first-hop guard and timer-driven expiry queue, respectively. Their threads remain unresolved, but the code fixes are present.
No new actionable findings in this delta. Direction: the ordered queues address both cleanup and bulk-drain cost without broadening the quarantine; the focused mitigation remains reasonable with its documented equal/better-rank limitation.
Verification: static review of the changed queue logic, cancellation/re-retraction matching, timer integration, unit test, and benchmark. I did not execute tests or benchmarks or independently verify the reported timings.
(Written by OpenAI)
|
Merge summary for head 2920eaa:
The same fixes are carried to the (Written by Claude Opus 5.5) |
Backport of the review fixes on #5375. Anchoring rescanned every unanchored retraction on each poll, so draining N retractions read at one clock reading took O(N^2). Queue retractions in clock order and pop only the due fronts, which also forgets ended quarantines without waiting for the map to double. An anonymous route names no publisher, so it no longer starts a quarantine. Adds origin/withdraw: 10000 retractions drain in 18 ms. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
backport: hold back a stale path to a prefix a cursor just retracted (#5375)
Summary
ENDthen fourSTART/ENDpairs within ~1.2 s, the hop count climbing 3 → 12 → 13 → 20.prune_withdrawn), and a cursor delivers a new route at once (update_holdonly holds changes). A relay further out, still path hunting, falls back to a stale path and offers it to a relay that already retracted, which announces it as new. fix(moq-net): hide routes through a peer that withdrew the prefix #4399 and fix(moq-net): hold route updates so a withdrawal retracts once #4642 don't cover this.update_holddisables it.retracted_relay_ignores_a_stale_path: an 8-relay mesh where relays 1 and 5 goEnd, Start, Endwithout the fix.a_retraction_quarantines_a_named_publisher; benchorigin/withdrawdrains N retractions read at one clock reading (10000 in 22 ms).Seeded random meshes (8-17 relays, links of 1-150 ms, one publisher withdrawing): relays that announce the broadcast again fall from 1042 to 15 across 200 meshes. The rest tie or beat the retracted route's rank after it had already hunted to a worse path. Removing them needs per-node seqnos (
quest/m1/cluster-routing), not more holds. Raisingupdate_holdalone makes it worse (stale alternatives live longer).API and wire impact
None. Behavior: a cursor may now announce a re-advertised prefix up to 2 s after it retracted it, only for a worse route to the same publisher instance.
Checks
2920eaa56:just checkpasses (6124 tests).🤖 Generated with Claude Code
(Written by Claude Opus 5.5)
Need help on this PR? Tag
@codesmith-botwith what you need.