feat(net): fail over across redundant publishers via per-peer route selection#2473
feat(net): fail over across redundant publishers via per-peer route selection#2473kixelated wants to merge 4 commits into
Conversation
![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
WalkthroughThe change adds optional fixed origin IDs to the CLI and documents redundant publisher operation. Broadcast state now retains complete ordered route tables with change polling. Origin routing carries per-consumer exclusions, filters tainted paths, and parks sources whose publisher identity differs. Lite publisher announcements select routes per peer, track serving paths, apply serving-only demand discounts, and use peer-aware broadcast resolution. Tests cover failover, split-horizon dispatch, route selection, and announcement retraction. 🚥 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.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@doc/bin/cli.md`:
- Around line 75-76: Update the Redundant Publishers reference in the --origin
documentation to use the heading’s actual fragment, `#redundant-publishers-11`, so
the in-page link resolves and satisfies MD051.
In `@drafts/draft-lcurley-moq-lite.md`:
- Line 298: Replace the em dash in the advertisement constraint sentence with a
colon or other permitted punctuation, preserving the existing meaning and
wording.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e9b13394-e2cb-4291-bed4-03501ed712af
📒 Files selected for processing (7)
doc/bin/cli.mddrafts/draft-lcurley-moq-lite.mdrs/moq-cli/src/args.rsrs/moq-cli/src/main.rsrs/moq-net/src/lite/publisher.rsrs/moq-net/src/model/broadcast.rsrs/moq-net/src/model/origin.rs
| /// when set, otherwise fresh and random. | ||
| pub fn origin(&self) -> anyhow::Result<moq_net::origin::Producer> { | ||
| Ok(match self.origin { | ||
| Some(id) => moq_net::Origin::new(id).map_err(|err| anyhow::anyhow!("--origin {id}: {err}"))?, |
TL;DRThanks for this — the per-peer selection design reads well and the model/wire unit tests
We don't think either is a reason to hold the PR (the mechanism it lands is a prerequisite Setup note: this PR predates #2469#2473 branched before #2469 ("linger a broadcast across an ungraceful source loss", Drill[cluster_failover.sh](https://github.com/user-attachments/files/30343689/cluster_failover.sh) Timeline: bring everything up; at t=10 Finding 1 — the standby route never reaches the relay serving the active sourceAcross every topology we tried (relay B dials A only; full mesh A↔B; and with/without a Our reading: the per-peer advertisement selection you added decides what to Is the intended design that the active-serving relay proactively holds all peer-advertised Finding 2 —
|
…election A relay announced only its single globally-best route per path, filtered per peer, so a standby publisher behind another relay was never presented to the node serving the active source; when that source died, the origin had nothing to reselect onto and playback froze (#2461). Even if the standby had been advertised, subscriptions always dispatched to the global active source, so a failover pull from the peer the active chain flows through would have looped. Announce and dispatch now share one selection: the best route whose hop chain excludes the requesting peer. The front mirrors its route table onto the spliced broadcast; each announce stream picks per exclude_hop (a peer inside the serving chain receives the best standby instead of nothing), and subscriptions from that peer are served by the same exclusion, which keeps advertised chains truthful and makes the exclude-hop filter loop-free at any cycle length. Content identity is the first hop: sources with a different first hop park until the incumbent ends instead of silently splicing, matching the session layer's restart rule. The warm cost discount now applies only to the serving route. moq-cli gains --origin to pin the publisher id, which is how 1+1 chains declare their content interchangeable. No wire format change; the behavioral rules are specified in draft-lcurley-moq-lite in the same change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
baeb69b to
0165ec9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rs/moq-net/src/model/origin.rs (1)
618-647: 🩺 Stability & Availability | 🔵 TrivialConfirm the shared-origin
Unroutableteardown is covered.Reviewers observed an
Unroutableteardown for a subscriber when a shared-origin standby joined a relay already carrying the broadcast from its peer (not seen with distinct origin IDs). That surfaces here: when the active chain contains the requester, resolution falls todispatch(Some(origin))and yieldsNone(→Unroutable) if every candidate route is tainted. The new tests cover the excluded-peer and all-tainted cases in the abstract, but not the shared-origin standby-join scenario the reviewers hit. Want me to draft a regression test reproducing that topology, or open an issue to track it?🤖 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 618 - 647, Add a regression test for consume_broadcast covering a shared-origin standby joining a relay already carrying the broadcast from its peer. Build the topology with the shared origin, relay, active peer route, and standby subscriber, then assert the standby remains routable and does not receive an Unroutable teardown; update consume_broadcast or dispatch only if the test exposes the described all-tainted failure.
🤖 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 618-647: Add a regression test for consume_broadcast covering a
shared-origin standby joining a relay already carrying the broadcast from its
peer. Build the topology with the shared origin, relay, active peer route, and
standby subscriber, then assert the standby remains routable and does not
receive an Unroutable teardown; update consume_broadcast or dispatch only if the
test exposes the described all-tainted failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a36e294-d829-47ad-8854-c8fbb192f01f
📒 Files selected for processing (7)
doc/bin/cli.mddrafts/draft-lcurley-moq-lite.mdrs/moq-cli/src/args.rsrs/moq-cli/src/main.rsrs/moq-net/src/lite/publisher.rsrs/moq-net/src/model/broadcast.rsrs/moq-net/src/model/origin.rs
| /// through the peer once its origin id is known, so a subscription is never | ||
| /// served data that flowed through the subscriber. | ||
| fn serving_origin(&self) -> origin::Consumer { | ||
| let peer = self.peer_origin.load(std::sync::atomic::Ordering::Relaxed); |
There was a problem hiding this comment.
This is a hack.
If we need the origin ID, then we need to get it some other way, like making it part of the SETUP message.
…UNCE_REQUEST Review feedback: the publisher learned the peer's origin id from the first ANNOUNCE_REQUEST's exclude_hop, which is per-stream metadata and absent for a peer that never opens an announce stream. Each endpoint now declares its Hop ID in an additive SETUP Origin parameter (0x5), filled in from the session's attached origin handles, and the serve-side split-horizon reads it from the shared peer-SETUP slot. Old peers ignore the unknown parameter. Also per review: --origin validation uses anyhow context, the cli.md section anchor is fixed, and a pre-existing em dash in the draft's announce prose is replaced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
That's not true, we always announce-interest to every peer. One of the problems is that we currently announce the best route to a peer, and we purposely skip doing it if the peer is in the hop list. If there's two valid routes (relay with 2 connections, each advertising a different "best" route), then we can fail to publish the better route to the peer. That being said, it doesn't explain the failure you're seeing, as relay A/B should both try to announce to each other.
I think we should only advertise a single route to a peer. Not only does this prevent explosive growth, but it also matches how SUBSCRIBE cannot choose a route. Like if we advertised 10 routes to a peer... it has no way of picking which one to use.
Yeah, we should splice if the first hop is shared.
Unit tests for both. |
| /// is never served data that flowed through the subscriber. Waits for the | ||
| /// peer's SETUP, which every lite-05+ endpoint sends at startup, well before | ||
| /// it could learn of anything to subscribe to. | ||
| async fn serving_origin(&self) -> origin::Consumer { |
There was a problem hiding this comment.
Cache this instead of recomputing it?
…ressions lite-06-wip removes ANNOUNCE_REQUEST's per-stream Exclude Hop; the SETUP Origin parameter is the session-wide identity that filters announcements and subscriptions alike. lite-04/05 keep the field on the wire. js/net mirrors both: AnnounceRequest carries excludeHop only on draft-04/05, and the client declares its origin in SETUP so relays keep suppressing reflected announces on lite-06. Unit tests for both findings from the #2461 e2e drill: a same-publisher standby attaching after the announce stream started goes out as a fresh announce to the excluded peer (and the active dying afterward is silent there), and a same-first-hop local standby joining a carrying front splices the live subscription instead of tearing it down. The dispatch test now also asserts the via-peer source receives no track request, which is the model-level image of "never forward a SUBSCRIBE back to the peer the data came from". A serving-flag flip with identical hops and cost no longer emits a redundant restart. Sharing an origin id is now documented as a promise: identical tracks with aligned group sequences, or use distinct ids and get replacement semantics. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@t0ms both publishers MUST be publishing identical content when using the same origin/name. If they're not, like it's two different publishers slurping the same TS feed, then the subscribers MUST get a signal to reinitialize on route change. A few ways of doing this:
|
… round The broadcast_route_migration integration test encoded the pre-identity contract: two publishers with different first hops migrating silently. Under the first-hop content-identity rule that is a replacement, not a migration, so the test now stamps a shared publisher id on both routes, which is the documented way to declare interchangeable content. This is what CI caught; local `just check` does not run the moq-native integration binaries. Review: cache the publisher's excluded origin handle (the peer sends exactly one SETUP per session, so the exclusion never changes). Update draft-lcurley-moq-relay-hops to match the lite model: the RELAY_HOPS Setup Option value now declares the endpoint's own Hop ID, replacing the EXCLUDE_HOP parameter; advertisement selection and subscription serving are per session, excluding the peer's declared id; same-first-entry advertisements are interchangeable (redundant publishers MAY share a Hop ID) while differing origins never splice. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Addresses #2461 (the mesh-propagation half; the remaining pieces are listed at the bottom, so this deliberately does not auto-close the issue).
Root cause
Active/active (1+1) source failover never worked across a mesh because the standby route was never presented to the node that needed it, and could not have been served even if it were:
broadcast.route()) and skips the announce entirely when that chain contains the peer'sexclude_hop(prepare_active_hops). So relay B, actively carryingpubA → relayA → relayB, advertised nothing to relay A even while holding a perfectly good standbypubB. Relay A's origin table then had nothing toreselectonto when pubA died: permanent freeze, exactly the repro in Active/active source failover: a standby publisher's route is never presented to the relay serving the active source (mesh), so a dead source is not failed over — even withmoq-lite-06cost routing #2461. Cost routing (feat(net): route by cumulative cost on lite-06 announcements #2424) could not help because pricing cannot rank a route that is never propagated.entry.publisher != publisher), but the same event across two sessions silently attached as a splice-compatible standby inattach_source, with no identity check at all.The fix
One selection function, used for both announcing and serving: the best route whose hop chain excludes the requesting peer.
lite/publisher.rs): the front mirrors its full route table (metadata only, preference-ordered, serving route first) onto the spliced broadcast (broadcast::Producer::set_routes, crate-private). Each announce stream selects from the table with its peer'sexclude_hop, so a peer the serving chain flows through receives the best standby instead of nothing. The winner changing per peer rides the existing restart machinery; swinging into/out of existence rides the existing filtered-announce arms, which already handled per-peer subsetting.model/origin.rs): sessions resolve broadcasts through a crate-privateorigin::Consumer::excluding(peer). The peer's identity arrives in its SETUP via a new additiveOriginparameter (0x5), declared by each endpoint at session start, so the exclusion applies even to sessions that never open an announce stream. When the active chain contains the requester, the lookup returns the best clean source directly instead of the shared front; when only tainted routes exist, the subscription is unroutable. Together with truthful advertised chains this is path-vector loop freedom: any would-be cycle surfaces the requester's own id inside a candidate chain, where the filter removes it — at any cycle length, which plain immediate-peer split horizon cannot guarantee.model/origin.rs):attach_sourcenow rejects a source whose first hop differs from the incumbent's. The rejected source parks invisibly and takes over the path once the incumbent ends (first wins, successor succeeds it), instead of silently splicing unrelated content into live subscribers. The session-level restart rule was already first-hop-keyed; this makes the cross-session path agree with it.run_sourceapplies the same check to route updates that change their own first hop.SentRoute.serving). A standby advertised to an excluded peer keeps its accumulated cost, because serving that peer means opening a fresh ingest. (This also fixes a busy-spin the naive version would have had: the demand watch armed on any nonzero sent cost, expecting the recompute to discount it to zero, which a standby never does.)moq --origin <id>(moq-cli): pins the process origin id (default: fresh random per run). Redundant 1+1 publishers share an id to declare their content interchangeable; a crash-restarted encoder keeps getting a fresh id and is correctly treated as new content. The relay already had this via--cluster-id. Sharing an id is a promise that both publishers produce exactly the same tracks with aligned group sequences (now spelled out indoc/bin/cli.md); independent publishers must use distinct ids, and the newcomer waits as a replacement.With this, the #2461 repro works end to end: relay B advertises
[pubB]to relay A; pubA dies; relay A splices to the standby at a group boundary; the restart that propagates keeps the first hop, so no downstream node tears anything down; relay A's subscription to relay B is dispatched to pubB even while relay B's own table still holds the stale via-relay-A entry (no loop window).Wire / draft
Two wire changes: a SETUP
Originparameter (0x5) declaring each endpoint's Hop ID at session setup (additive: SETUP receivers ignore unknown parameter IDs, so it is backward-compatible with deployed lite-05 peers), and lite-06-wip drops ANNOUNCE_REQUEST's per-streamExclude Hopin its favor (lite-06 is unreleased and negotiates only by explicit opt-in, so this breaks no published contract and the PR still targetsmain). Lite-04/05 keep the field on the wire and behave exactly as before. The normative changes are specified indrafts/draft-lcurley-moq-lite.mdin this PR: the Origin parameter, per-subscriber advertisement selection, the serve-by-the-same-rule requirement, shared-first-hop interchangeability (redundant publishers MAY share a Hop ID), and the serving-only scope of the carrying discount.Behavioral changes to existing public APIs (no signature changes)
origin::Producer::create_broadcast: a second source at a path with a different first hop no longer joins as a standby; it parks until the incumbent ends. Sources with the same first hop (or both hopless/local) behave exactly as before.Cross-package sync
js/net: mirrored the wire changes.AnnounceRequestcarriesexcludeHoponly on draft-04/05, and the client declares its session origin via the SETUPOriginparameter so relays keep suppressing reflected announces on lite-06. No selection/dispatch logic to mirror: JS is a leaf with no multi-route origin table.moq-ffi/ bindings: no surface change. Exposing an origin-pin knob to bindings publishers is a natural follow-up but not needed for the core mechanism.moq-relay: no change;--cluster-idalready pins the relay origin.doc/bin/cli.mdgains--originand a "Redundant Publishers (1+1)" section.Tests
test_publisher_mismatch_parks: different first hop parks, takes over on incumbent end as unannounce + announce (also regression-covers the orphaned-leaf bug found while writing it: a parked source now re-resolves its tree leaf per attach attempt, since the incumbent's teardown can prune it).test_dispatch_excludes_requester: tainted-for-the-peer active → peer pinned to the clean source, everyone else on the shared front.test_dispatch_all_tainted_unroutable: all routes through the requester →Unroutable.excluded_peer_receives_the_standby(wire): peer in the active chain receives the standby chain at its undiscounted cost while an ordinary peer receives the active chain at the warm discount.retracts_when_route_swings_through_peer(wire): route swinging into the peer's chain →ANNOUNCE_END; swinging back → freshANNOUNCE_START.standby_attach_announces_to_excluded_peer(wire): a relay carrying via its peer initially advertises nothing to them; a same-publisher standby attaching later goes out as a fresh announce, and the active source dying afterward is invisible on that stream (the standby is already advertised). Unit shape of the e2e drill's finding 1.test_standby_join_splices_live_subscriber: a local same-first-hop standby joining a front that is carrying from a peer splices the live subscription at the group boundary (finding 2, with the aligned content that a shared origin id requires).test_dispatch_excludes_requesternow also asserts the via-peer source receives no track request at all: a track request on that source is exactly what a session would forward upstream as a SUBSCRIBE, so this is the "never subscribe back to the peer the data came from" guarantee.announce_request_exclude_hop_by_version+ js mirrors: the field round-trips on lite-04/05 and is absent from lite-06's wire image.Not in this PR
Follow-ups toward the full #2461 acceptance test: an end-to-end two-relay integration drill (the issue authors offered their
cluster_failover.shas a starting point), an origin-pin knob for the bindings/gateways (moq-ffi, gst, rtmp/srt ingest), and subscriber multi-homing (option B in the issue).(Written by Fable 5)