fix(net): only a new publisher re-arms the origin takeover gate - #2746
Conversation
A standby source could evict the live front with a cost-only repricing. Any route observation re-armed `may_take_over`, and `lite::subscriber` calls `set_route` on every same-publisher re-announce, so a peer repricing a link handed a source that had already lost the path a fresh takeover attempt. Two rivals at one path then trade the front on metadata churn, and each trade unannounces and cuts the winner's subscribers. Cost is not content. Ownership of a path is decided by publisher identity; cost only orders routes within a front, via `route_order` and `reselect`. So re-arm the gate on a first-hop change alone. Plain equality, matching the detach check in the same loop, so an UNKNOWN-to-UNKNOWN repricing from a legacy peer proves no new identity and does not re-arm either. A source that never lost the path is unaffected: the flag starts set and only a displacement clears it, so an offline source going announced still takes over on the strength of the gate it was already holding. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for security reviews. Please try again later. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughTakeover eligibility now re-arms only when a route’s first hop changes. Same-publisher cost and metadata updates no longer re-arm a displaced source, including 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rs/moq-net/src/model/origin.rs (1)
5000-5045: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
UNKNOWN-to-UNKNOWNrepricing.The current test uses two known first-hop IDs. It does not exercise the special equality case on Lines 1613-1616. Add a case where both displaced and live sources have
Origin::UNKNOWNas their first hop, then reprice the displaced source and assert that no unannounce or announce occurs.As per coding guidelines, "Reproduce bugs, identify and state the root cause, fix the lowest responsible layer, and add a regression test that fails without the fix."
(Written by CodeRabbit)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-net/src/model/origin.rs` around lines 5000 - 5045, Extend test_repricing_does_not_earn_a_takeover with a scenario where both displaced and live routes use Origin::UNKNOWN as their first hop. Reprice the displaced source, then assert the current broadcast remains live and announced emits neither an unannounce nor an announce event, covering the UNKNOWN-to-UNKNOWN equality handling.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rs/moq-net/src/model/origin.rs`:
- Around line 5000-5045: Extend test_repricing_does_not_earn_a_takeover with a
scenario where both displaced and live routes use Origin::UNKNOWN as their first
hop. Reprice the displaced source, then assert the current broadcast remains
live and announced emits neither an unannounce nor an announce event, covering
the UNKNOWN-to-UNKNOWN equality handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 97749b49-43d2-426a-b07c-f48439f26db6
📒 Files selected for processing (1)
rs/moq-net/src/model/origin.rs
Co-Authored-By: OpenAI Codex <noreply@openai.com>
1 participant
Summary
Follow-up to #2740 and #2742, from testing a residual concern raised while reviewing them.
may_take_over, andlite::subscribercallsset_routeon every same-publisher re-announce (rs/moq-net/src/lite/subscriber.rs, theentry.set_route(metadata)arm), so a peer repricing a link handed a source that had already lost the path a fresh takeover attempt.maintoday: A is displaced by B, A's peer re-announces at a new cost, and A takes the path back.route_orderandreselect. So re-arm the gate on a first-hop change alone.UNKNOWN-to-UNKNOWNrepricing from a legacy peer proves no new identity and does not re-arm either.A source that never lost the path is unaffected: the flag starts set and only a displacement clears it, so an offline source going announced still takes over on the strength of the gate it was already holding.
Public API changes
None.
run_sourceandattach_sourceare private.Cross-package sync
Not applicable. No wire format or public API change, and
js/nethas no origin front/routing implementation to mirror.Test plan
test_repricing_does_not_earn_a_takeovercovers known publisher identities. It fails without the guard withleft: OriginList([Origin { id: 1 }]), right: OriginList([Origin { id: 2 }]).test_unknown_publishers_do_not_splicealso coversUNKNOWN-to-UNKNOWNrepricing. It fails without the guard atUNKNOWN-to-UNKNOWN repricing must not replace the live front.nix develop --command cargo nextest run -p moq-net(718 passed)nix develop --command just ci(2,823 passed, 13 skipped; clippy, formatting, docs, dependency policy, WASM, Nix flake, and workflow checks passed)(Written by GPT-5)