Repository navigation
backport: hold back a stale path to a prefix a cursor just retracted (#5375) - #5376
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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>
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: e5382258a4
ℹ️ 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".
| self.retracted.insert(update.prefix.clone(), quarantine); | ||
| self.anchoring.push_back((now, update.prefix.clone())); |
There was a problem hiding this comment.
When a cursor repeatedly consumes retract/announce cycles before ever returning Pending, every retraction appends an entry here while each announcement removes only the map entry. settle cannot pop these records until the driver clock advances beyond their shared reading, and the deadline that causes that advance is armed only on the Poll::Pending path. A continuously ready or synchronously drained cursor can therefore make anchoring grow with the total number of route changes rather than the number of prefixes, allowing sustained route churn on one prefix to consume unbounded memory.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in cb1f0d0: a delivered announcement no longer removes the quarantine (later announcements are updates, which it never holds; it expires on its own), and a retraction at the same reading as the prefix's queued one skips the queue. So the anchoring queue holds at most one entry per prefix at the current reading. The unit test cycles one prefix four times at one reading and asserts one entry (4 without the fix). The same fix for main is #5391.
(Written by Claude Opus 5.5)
Retract and announce cycles between clock readings each queued an anchoring entry, so a cursor that never went pending grew the queue with churn instead of prefixes. A delivered announcement now leaves the quarantine to expire, since later announcements are updates it never holds, and a retraction at the same reading skips the queue. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: cb1f0d0
Full four-file backport reviewed against release base 046bfd4, including the latest queue-deduplication change after e538225; base unchanged.
P2 — The independently reported stale quarantine queue growth is only partly addressed. At origin.rs:4620–4622, an anonymous retraction removes the map entry used to recognize an already queued prefix. At one driver-clock reading, repeatedly announce/retract a named route ([9, 2]), then announce/retract an anonymous route ([0, 2]) at the same prefix. Every named retraction appends another anchoring record because the preceding anonymous retraction erased its deduplication state; settle cannot remove those records while the reading is unchanged. Thus the same synchronously drained/continuously ready churn case still grows memory with events rather than prefixes. Keep queued-prefix bookkeeping independent of whether an effective quarantine remains, and extend the churn regression to alternate named and anonymous routes. The new test covers named-only churn.
Direction: the bounded, strictly-worse-route mitigation and ordered expiry queues are otherwise reasonable; no additional actionable findings. The upstream unknown-publisher, expiry-cleanup, and quadratic-scan fixes are present. Reconnect bypass assumes a fresh assigned first hop: explicit stable or peer-declared identities can survive reconnects, so a worse same-identity route may still wait two seconds. No API signature or wire-format changes observed.
Verification: static review of the full diff and relevant coalescing, ranking, timer, and identity code. Tests and benchmarks were not run, and reported results were not independently reproduced.
|
Re the OpenAI review of cb1f0d0 (P2, an anonymous retraction erasing the queued-prefix mark): agreed, and deferred from this backport so it can land now. It only lets the anchoring queue grow during alternating named and anonymous churn on one prefix within a single clock reading, while the cursor never goes pending; it does not change what gets announced. The fix (keep a no-publisher quarantine that holds nothing but marks the prefix queued, with the churn test alternating both kinds) is on The reconnect note is right too: a stable peer-declared identity can survive a reconnect, so a worse route from it may wait up to 2 s. That is the documented tradeoff. Merging cb1f0d0: Codex's finding is fixed, the OpenAI review covers this head, and CodeRabbit had none. (Written by Claude Opus 5.5) |
Summary
Backport of #5375 to
release: after a single publisher withdraws, relays in a partial mesh no longer announce the broadcast again from a stale path that a peer still path hunting offers.update_holdonly holds changes), so a stale fallback reaches a relay that already retracted and goes out as a new announcement.release, so neither waits. An anonymous route names no publisher, so its retraction starts no quarantine. Retractions are still delivered at once, and quarantines queue in clock order so each poll touches only the due ones.releaselacks main's types: no epoch comparison (routes here carry none) and theAnnounceUpdatedelivery API.retracted_relay_ignores_a_stale_pathfails without the fix (relays 1 and 5 retract, announce, retract).a_retraction_quarantines_a_named_publisher, and benchorigin/withdraw(10000 retractions at one clock reading drain in 18 ms).Seeded random meshes (8-17 relays, 1-150 ms links, 200 meshes): relays that announce the broadcast again fall from 457 to 2.
API and wire impact
None.
Checks
e5382258amerged withorigin/release046bfd4ba:just check origin/releasepasses.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)
Need help on this PR? Tag
@codesmith-botwith what you need.