fix(net): prefer the newest route so a reconnect takes over immediately - #2556
Conversation
A publisher reconnecting could not reclaim its path until the transport retired the session it replaced, which for an ungraceful disconnect means the QUIC idle timeout. Two separate mechanisms preferred the older route, depending on whether the reconnect keeps its origin id. Same origin id (a Rust publisher, or an upstream relay): both sessions attach as routes to one front, but their hop chains and costs are identical, so route_order tied exactly and min_by_key returned the first inserted. Recency is now the final tie-break, below the hop hash, so it only separates routes that are indistinguishable to the rest of the mesh. Different origin id (js/net mints one per connection, so every browser reconnect): the newcomer never entered the table at all. attach_source parked it on a first-hop mismatch, waiting out the incumbent's idle timeout plus the origin linger. It now closes the incumbent front and falls through to the existing replacement path, so consumers observe an unannounce followed by an announce. That preserves the guarantee the park was protecting (unrelated content is never spliced into a live subscription) while making the swap immediate. The consequence is that path ownership is last-writer-wins: a publisher announcing a path a live different publisher holds evicts them rather than queueing behind them. Nothing can distinguish "dead but not yet timed out" from "alive" at attach time, so preferring the newest requires it. Authorization decides who may publish where, and a takeover logs a warning. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @kixelated, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
WalkthroughRoute selection now uses attachment recency as a final tie-breaker across route selection and snapshots. Origin broadcast attachment closes an announced incumbent and returns a new broadcast immediately when publisher identities differ, while offline mismatches remain parked. Tests were updated for replacement, parking, and reconnect behavior. CLI and draft protocol documentation now describe recent-advertisement tie-breaking and later-advertisement replacement. 🚥 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rs/moq-net/src/model/origin.rs (2)
1555-1564: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
attach_sourcehas 7 positional parameters.Per path instructions for
rs/**/*.{rs,ts,tsx,js,jsx}, functions with four or more arguments should be refactored into a struct "while changing code" — and this function's join/replace logic was just rewritten.origin,node,full, andrestare all invariant per-call context carried fromrun_source; bundling them (e.g. anAttachContextstruct) would cut this to 4 params and reduce the chance of a future misordered-argument bug at the (currently single) call site.Based on path instructions: "Refactor awkward internal shapes while changing code: replace functions with four or more arguments or repeated groups of values with appropriate structs or abstractions."♻️ Sketch
-fn attach_source( - origin: &Info, - node: &Lock<OriginNode>, - leaf: &Lock<OriginNode>, - full: &PathOwned, - rest: &PathOwned, - source: &broadcast::Consumer, - route: broadcast::Route, -) -> (kio::Producer<FrontState>, broadcast::Producer, u64) { +struct AttachContext<'a> { + origin: &'a Info, + node: &'a Lock<OriginNode>, + full: &'a PathOwned, + rest: &'a PathOwned, +} + +fn attach_source( + ctx: &AttachContext, + leaf: &Lock<OriginNode>, + source: &broadcast::Consumer, + route: broadcast::Route, +) -> (kio::Producer<FrontState>, broadcast::Producer, u64) {🤖 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 1555 - 1564, Refactor attach_source to accept an AttachContext struct bundling the invariant origin, node, full, and rest values, reducing its positional arguments to four or fewer. Update the run_source call site and all references inside attach_source to use the context fields, preserving the existing join/replace behavior and argument values.Source: Path instructions
1543-1602: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
FrontState.closedfield doc is now incomplete.
attach_sourcegains a third way to setclosed = truehere: an immediate close on publisher mismatch. Theclosedfield's own doc comment (lines 1232-1235, unchanged in this diff) still only documents two triggers ("the detach that empties the table, or ...run_frontwhen the linger window expires"), not this new reconnect-replacement trigger. A reader relying on that field-level doc to understandclosed's terminal semantics would miss this path entirely.Based on path instructions: "Comments and documentation must describe the current behavior, not historical migration context or obsolete behavior."📝 Suggested doc update (at the `closed` field, ~line 1232)
/// Terminal: no more sources may attach and every poller stops. Set - /// synchronously by the detach that empties the table, or by [`run_front`] - /// when the linger window expires without a replacement. + /// synchronously by the detach that empties the table, by [`run_front`] + /// when the linger window expires without a replacement, or by + /// [`attach_source`] when a newcomer with a different publisher replaces + /// this front immediately. closed: bool,🤖 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 1543 - 1602, Update the documentation for the FrontState.closed field to include immediate closure when attach_source detects a different publisher and replaces the live broadcast. Keep the existing detach and linger-expiry triggers documented, and describe closed as covering all terminal shutdown paths without historical or migration-specific context.Source: Path instructions
🧹 Nitpick comments (1)
rs/moq-net/src/model/origin.rs (1)
4219-4304: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider a test for reconnect during an active (carrying) subscription.
test_reconnect_wins_over_stale_routeattaches the fresh session before any subscriber exists, soreselectruns withcarrying=falseand simply takesbest_route(). The more interesting case — a reconnect arriving while a subscriber is already spliced onto the stale route — exercises the interaction between the new recency tie-break and the existing simultaneous-activation-race gate inreselect(the gate is bypassed here because tied costs fail its strictcost <check, but that interaction isn't covered by a test). A regression test pinning that behavior would guard against a future change to the gate accidentally re-introducing a wait for tied reconnects.🤖 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 4219 - 4304, Add a regression test alongside test_reconnect_wins_over_stale_route that establishes a subscriber and active track on the stale session before creating the fresh same-identity route, then verify reselect immediately switches the carrying subscription to the fresh route despite tied costs. Confirm subsequent track requests reach the fresh session, the stale session receives no new request, and the subscriber remains functional without waiting for transport timeout.
🤖 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.
Outside diff comments:
In `@rs/moq-net/src/model/origin.rs`:
- Around line 1555-1564: Refactor attach_source to accept an AttachContext
struct bundling the invariant origin, node, full, and rest values, reducing its
positional arguments to four or fewer. Update the run_source call site and all
references inside attach_source to use the context fields, preserving the
existing join/replace behavior and argument values.
- Around line 1543-1602: Update the documentation for the FrontState.closed
field to include immediate closure when attach_source detects a different
publisher and replaces the live broadcast. Keep the existing detach and
linger-expiry triggers documented, and describe closed as covering all terminal
shutdown paths without historical or migration-specific context.
---
Nitpick comments:
In `@rs/moq-net/src/model/origin.rs`:
- Around line 4219-4304: Add a regression test alongside
test_reconnect_wins_over_stale_route that establishes a subscriber and active
track on the stale session before creating the fresh same-identity route, then
verify reselect immediately switches the carrying subscription to the fresh
route despite tied costs. Confirm subsequent track requests reach the fresh
session, the stale session receives no new request, and the subscriber remains
functional without waiting for transport timeout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c82a56d-3d94-4bae-84cb-71eeb001ef94
📒 Files selected for processing (5)
doc/bin/cli.mddrafts/draft-lcurley-moq-lite.mddrafts/draft-lcurley-moq-relay-hops.mdrs/moq-net/src/model/broadcast.rsrs/moq-net/src/model/origin.rs
|
Confirmed that this also resolves the restarted-numbering/stitching issue investigated in #2534. When the replacement's first hop changes, it is now surfaced as a new broadcast instead of being stitched at the incumbent's I tested head (Written by GPT-5) |
The takeover added in the previous commit keyed only on first-hop identity, so an offline source (Route::new(), the shape a cache or on-demand handler uses) with a different publisher id would close a live announced front, unannounce the path, and cut its subscribers. That contradicts the route ordering the takeover is supposed to extend, where an announced route outranks every offline one. Taking over now requires announcing. An offline newcomer parks until the incumbent closes, which is the pre-existing behavior for that case. The rule is a single statable invariant: a source only displaces a path it would outrank, and a reconnecting publisher always announces, so the reconnect fix is untouched. Also fold attach_source's invariant arguments into an AttachContext (it carried 7 positional parameters) and correct the FrontState docs, which still described closed as having only two triggers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rs/moq-net/src/model/origin.rs (1)
1459-1478: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a quieter log level for routine reconnect takeovers/parks.
Both
tracing::warn!calls fire whenever a source's first hop differs from the live front's — the offline-park case (Line 1507-1510) and the announced-replace case (Line 1670). GivenOrigin::random()is the default per-connection identity (seers/moq-cli/src/args.rs'sorigin()), a basic client reconnect without a pinned--originwill commonly present a different first hop each time, making this the routine reconnect path rather than an anomaly. Atwarn!, a relay handling frequent reconnects could generate substantial log noise for expected behavior.info!ordebug!would better reflect that this is normal operation.Also applies to: 1507-1510, 1670-1670
🤖 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 1459 - 1478, Lower the log level of the routine first-hop mismatch messages in run_source, including both the offline-park case and the announced-replace takeover case, from warn to info or debug. Keep the existing messages and behavior unchanged.
🤖 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 1459-1478: Lower the log level of the routine first-hop mismatch
messages in run_source, including both the offline-park case and the
announced-replace takeover case, from warn to info or debug. Keep the existing
messages and behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bf0d220e-c18b-4a51-8704-926af2dcb7f4
📒 Files selected for processing (1)
rs/moq-net/src/model/origin.rs
Planting a panic in the park's wake path left all 602 tests green: the old test_publisher_mismatch_parks covered park-then-take-over, and repurposing it into the takeover test dropped that coverage. A lost wakeup there would strand a publisher invisibly forever rather than fail anything, which is the class of bug loom is a manual gate for. Extend test_offline_mismatch_never_evicts_a_live_front through the incumbent ending, so the wait is driven end to end. Verified by mutation: stubbing the incumbent poll to Pending now fails it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3992aae to
6e2933c
Compare
Two text conflicts, both additive: - `Route::cost`: #2556 added recency as a fourth tie-break and documented it, while this branch documented the `MAX_COST` cap. Kept both. - The moq-lite changelog: dev reworded the splice bullet ("the later replacing the earlier"), this branch added two GOAWAY bullets. Kept dev's wording plus both bullets. `route_order` now takes a `FrontRoute` and ends in `Reverse(id)`, so `drain_cost_sorts_last` was updated for the signature. It now gives the draining route the highest id in every comparison, which is the case recency introduced: a reconnect should take over immediately, but not when the newcomer is the one going away. Cost is the second term, so DRAIN_COST still dominates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
Summary
A publisher reconnecting could not reclaim its path until the transport retired the session it replaced. For an ungraceful disconnect that is the QUIC idle timeout (30s by default), which is the failover-latency caveat called out at the bottom of #2473. Two separate mechanisms preferred the older route, depending on whether the reconnect keeps its origin id:
route_ordertied exactly andmin_by_keyreturned the first inserted, i.e. the corpse. Recency (Reverse(FrontRoute::id), the front's attach counter) is now the final tie-break, below the hop hash, so it only separates routes that are indistinguishable to the rest of the mesh. Local attach order never leaks into cluster convergence: what gets forwarded is the chain and cost, equal by construction wherever it applies.js/netmints one per connection, so every browser reconnect): the newcomer never entered the table at all.attach_sourceparked it on a first-hop mismatch and waited out the incumbent's idle timeout plus the origin linger. An announced newcomer now closes the incumbent front and falls through to the existing stale-entry replacement path, so consumers observe an unannounce followed by an announce. That preserves the guarantee the park was protecting (unrelated content is never spliced into a live subscription) while making the swap immediate.Taking a path over requires announcing, which keeps the takeover consistent with the ordering it extends: an offline source (
Route::new(), the shape a cache or on-demand handler uses) ranks below every announced route, so it still parks rather than unannouncing a live broadcast and cutting its subscribers. The second commit fixes that; the first commit alone would let an offline different-publisher source evict a live front, whichtest_offline_mismatch_never_evicts_a_live_frontreproduces.Behavioral change worth reviewing
Path ownership is now last-writer-wins among announced publishers: one announcing a path that a live different publisher holds evicts them rather than queueing behind them. Nothing can distinguish "dead but not yet timed out" from "alive" at attach time, so preferring the newest requires this. Authorization is what decides who may publish where; a takeover logs a warning. Redundant 1+1 publishers are unaffected, since they share an origin id and remain standbys.
The displaced source is orphaned rather than re-queued: it does not reclaim the path if the usurper later leaves. That is deliberate, since re-attaching on eviction would let two live publishers evict each other in a loop.
Public API changes
None. No signature changes;
route_order,attach_source,Attach, andAttachContextare private.origin::Producer::create_broadcastchanges behavior only (an announced different-first-hop source replaces instead of parking), which reverses the corresponding bullet in #2473. Targetsmainper the branch-targeting rules.Wire / draft
No wire-format change. Both drafts normatively specified the old rule and are updated:
draft-lcurley-moq-lite.mdanddraft-lcurley-moq-relay-hops.mdnow specify the recency tie-break and later-replaces-earlier instead of "keep serving the earlier one until it ends". Both are scoped to advertisements, which matches the announce requirement above. lite-06 is unreleased, so its changelog bullets are amended in place rather than appended.Cross-package sync
drafts/: both drafts above.doc/bin/cli.md: the "Redundant Publishers (1+1)" section said a different-id publisher waits invisibly; it now describes the takeover and points at authorization for path ownership.js/net: no mirror needed. It is a leaf with no multi-route origin table, and nothing on the wire changed.Test plan
test_reconnect_wins_over_stale_route: a same-id reconnect attaches alongside the stale route, and track requests dispatch to the new session while the stale source receives none.test_carrying_reconnect_switches_immediately: the same reconnect arriving while a subscription is already spliced onto the stale route, which re-splices onto the new session.test_offline_mismatch_never_evicts_a_live_front: an offline different-publisher source at a path with a live announced front and an active subscriber leaves both intact.test_publisher_mismatch_replaces(wastest_publisher_mismatch_parks): the announced takeover is immediate, is a distinct face rather than a splice, and the displaced front is closed rather than merely unpublished.test_carrying_switches_to_benign_routesgains a tied-cost reconnect case, pinning thatreselect's simultaneous-activation gate stays strictly-cheaper.test_offline_mismatch_never_evicts_a_live_frontalso drives the park's exit: the incumbent ends and the parked source takes over. A review pass caught that nothing covered this (a panic planted in the wake path left all 602 tests green), which matters because a lost wakeup there strands a publisher invisibly rather than failing anything.Reverse(route.id), droppings.closed = true, dropping the!route.announcepark, stubbing the park's incumbent poll toPending, and loosening the gate to<=each fail exactly one test. (The eviction was invisible until theis_closedassertion was added, since the replacement path fires either way.)just checkclean;cargo test -p moq-net602 passing;just rs loompasses (this touchesmoq-net/src/model/).Tooling note: finding the park gap took two runs that hung and pegged a core, because
cargo testhas no timeout. That fix (nextest by default, plus anopt-leveloverride that takes the RSA keygen tests from 16s to 0.8s) is split out into #2558 rather than bundled here.Review notes
CodeRabbit's two maintainability findings are addressed in the second commit (
attach_sourcedown to 4 parameters viaAttachContext; theFrontState::closeddoc now lists all three triggers), as is its suggested carrying-reconnect test. Its premise for that test was half right: for a single-hop publisher chain the gate is unreachable onhops.len() >= 2regardless of the cost comparison, so the<boundary is pinned by the unit-level case intest_carrying_switches_to_benign_routesinstead.(Written by Opus 5)