test(failover): two-relay 1+1 source-failover drill - #2545
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds a two-relay active/active failover acceptance test with configurable source, idle timeout, ports, profile, and log retention. The harness builds required binaries, starts a meshed relay pair, launches three subscribers and two origin-tagged publishers, kills publishers on a timed schedule, samples output sizes, and grades failover and standby-join survival. It also adds README documentation covering topology, timing, prerequisites, usage, and debugging, plus a 🚥 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: 6
🤖 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 `@test/failover/README.md`:
- Line 8: Remove all em dashes from the prose in test/failover/README.md,
including the referenced passages around “this covers the wiring,” “for that
work,” “expires,” “reselect itself,” and “behave very differently.” Reword or
punctuate those sentences with commas, periods, colons, or parentheses while
preserving their meaning.
- Around line 14-18: Add the text language identifier to the fenced diagram code
block in the README, changing the opening fence to use text while preserving the
diagram content unchanged.
- Line 93: Update the prerequisites list in the failover README to include pkill
alongside the tools already required by run.sh’s require_tools function,
including cargo, ffmpeg, curl, pgrep, and TSDuck’s tsp.
In `@test/failover/run.sh`:
- Around line 24-27: Update the CHECK 2 comment in the standby-join test to
describe the current expected behavior and why sub3 on relayB must survive pubB
joining, removing the historical reference to the former Unroutable subscription
failure.
- Around line 38-39: Update the runtime estimates in the comments for the
default and --idle 10s invocations in run.sh to match the documented timeline:
approximately 90 seconds for the default 30-second budget and approximately 70
seconds for the 10-second budget.
- Around line 76-82: The IDLE_BUDGET derivation in the failover script
incorrectly strips only a trailing “s” and cannot parse humantime durations.
Replace this conversion with the existing duration-class or equivalent humantime
parser to convert valid IDLE values such as milliseconds, compound durations,
and bare seconds into whole seconds before timestamp arithmetic; retain the
30-second fallback when IDLE is unset.
🪄 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: a743ed33-2395-4235-ab0e-20feef2542a4
📒 Files selected for processing (3)
test/failover/README.mdtest/failover/run.shtest/justfile
| the surviving standby takes over. | ||
|
|
||
| This is the end-to-end counterpart to the routing unit tests in | ||
| `rs/moq-net/src/model/origin.rs`. Those cover the model; this covers the wiring — |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Em dashes used in prose — violates the no-em-dash guideline.
Found at "this covers the wiring —" (line 8), "for that work — it fails" (line 27), "expires — 30 s by default" (line 61), "reselect itself — on this checkout" (line 69), and "behave very differently — on a clean detach" (line 81).
✏️ Suggested rewording
-`rs/moq-net/src/model/origin.rs`. Those cover the model; this covers the wiring —
-two real `moq-relay` processes, real `moq import ts` publishers whose tracks are
+`rs/moq-net/src/model/origin.rs`. Those cover the model; this covers the wiring:
+two real `moq-relay` processes, real `moq import ts` publishers whose tracks are
created lazily by the demuxer, and the QUIC idle timeout in the loop.-> for that work — it fails on `main` for want of the flag, and both checks pass
+> for that work; it fails on `main` for want of the flag, and both checks pass
> on `#2473`'s head.-the dead source until the **QUIC idle timeout** expires — 30 s by default. The
+the dead source until the **QUIC idle timeout** expires (30 s by default). The-not by the reselect itself — on this checkout `sub1` resumes 30–33 s after the
+not by the reselect itself: on this checkout `sub1` resumes 30–33 s after the-detach rather than a source failure. Worth knowing because the two paths behave
-very differently — on a clean detach the subscriber's `export ts` currently dies
+detach rather than a source failure. Worth knowing because the two paths behave
+very differently: on a clean detach the subscriber's `export ts` currently diesAs per coding guidelines, "Do not use em dashes in code, comments, documentation, commit messages, or prose."
Also applies to: 27-27, 61-61, 69-69, 81-81
🤖 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 `@test/failover/README.md` at line 8, Remove all em dashes from the prose in
test/failover/README.md, including the referenced passages around “this covers
the wiring,” “for that work,” “expires,” “reselect itself,” and “behave very
differently.” Reword or punctuate those sentences with commas, periods, colons,
or parentheses while preserving their meaning.
Source: Coding guidelines
| # Detection budget follows the relays' idle timeout when it is pinned, otherwise | ||
| # quinn's 30s default. | ||
| if [[ -n "$IDLE" ]]; then | ||
| IDLE_BUDGET="${IDLE%s}" | ||
| else | ||
| IDLE_BUDGET=30 | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what format moq-relay's --server-quic-idle-timeout expects/parses.
rg -n "quic-idle-timeout" --type=rust -C6Repository: moq-dev/moq
Length of output: 149
Avoid deriving IDLE_BUDGET when --idle uses human-time parsing.
--server-quic-idle-timeout accepts humantime durations, so 500ms, 1m30s, 30s and bare seconds can all be valid relay arguments. ${IDLE%s} turns those into non-seconds like 500m or 1m30, causing the numeric timestamp arithmetic to fail badly. Convert IDLE to whole seconds via the same duration class or a matching parser instead of stripping a trailing s.
🤖 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 `@test/failover/run.sh` around lines 76 - 82, The IDLE_BUDGET derivation in the
failover script incorrectly strips only a trailing “s” and cannot parse
humantime durations. Replace this conversion with the existing duration-class or
equivalent humantime parser to convert valid IDLE values such as milliseconds,
compound durations, and bare seconds into whole seconds before timestamp
arithmetic; retain the 30-second fallback when IDLE is unset.
Re-tested source failover on the #2473 head: it works, bounded by one QUIC idle timeout (30-33s), and recovery is complete and repeatable once the publisher pipeline is killed atomically. This retracts our earlier "inconsistent recovery" caveat, which came from a drill that killed tsp first and so graded a graceful detach rather than a source failure. Two gaps now documented: the standby join stalls carrying-relay viewers 8-9s, and a graceful source exit is not failed over at all (the subscriber terminates on the catalog change). Both reported upstream; the corrected drill is contributed as moq-dev/moq#2545. Keeps the conclusion unchanged: failover is bounded, not hitless, so receiver-side ST 2022-7 / IRD selection stays the broadcast-grade path.
Stands up two meshed relays, a redundant publisher pair sharing an origin id, and three subscribers, then kills the active source and checks that the standby takes over. The end-to-end counterpart to the routing unit tests in model/origin.rs: real relays, real publishers whose tracks are created lazily by the demuxer, and the QUIC idle timeout in the loop. Two graded checks. Failover: sub1 on relayA must resume after pubA dies, which requires relayB to have already advertised the local pubB standby to relayA. Standby join: sub3 on relayB must keep its subscription when pubB attaches locally, the case a model-level standby cannot reproduce because it accepts every track request immediately whereas a real publisher has not created every track yet. The subscriber on relayB is load-bearing rather than decorative. It forces relayB to carry the broadcast via relayA, which is what makes relayA a hop in relayB's route and therefore the peer that per-peer announce selection has to treat specially. Three properties of the harness decide whether a run means anything, so each is documented where it is set. The observation window is derived from the QUIC idle timeout rather than chosen, because killing a publisher sends no CONNECTION_CLOSE and the relay serves the dead source until that timeout expires: a shorter window cannot pass on any build. The kill takes the whole pipeline down in one pass, because killing tsp first lets moq import see EOF and shut the broadcast down cleanly, which grades a graceful detach instead of a source failure. And the standby joins early, because the two publishers replay independent copies of the same clip from its start: their media timelines differ by the join delay, so the subscriber's muxer waits for the new source's timestamps to overtake the last ones it wrote, one second per second of join delay. That wait belongs to the harness, not the relay, which reselects in the same millisecond the standby connects. On current main both checks pass: sub1 resumes 30 to 33s after the kill, one idle timeout, so the reselect itself is essentially free. Kept out of the default `just test` aggregate, like `just test smoke`.
5f8deb8 to
e8d4d19
Compare
|
Rebased on merged One of the two things this drill reported was the drill's own fault, and it is gone. The CodeRabbit's review, addressed:
Also exposed the join and kill times as
|
|
With the timeline stuff, if you're publishing two separate broadcasts (ie. different timestamps, different codecs, different segments, etc) with the same name then failover is impossible. We need explicit cooperation from publishers to produce the exact same broadcast. If they're not the same, then they could be published with different names. foo/primary and foo/backup for example. Either way the application needs to reinitialize everything on a switch. |
|
Following up with data. We rebuilt the drill around one source feeding both publishers, since a 1+1 pair should be two views of one feed rather than two independent encodes. On current main (moq-relay 0.14.3):
So in our testing the requirement was a common source (same PMT/track layout, consistent PTS), not byte-identical segmentation, the consumer resumes at the new live edge rather than demanding group-number continuity. The switch is not hitless: the ~30 s outage is a PCR/PTS discontinuity (continuity-counter stays clean, but there's a content hole). Lowering --server-quic-idle-timeout to 10 s gives an ~11 s switch; 5 s was too aggressive and the switch didn't complete. Is a lower idle default on the table? |
So I think this requirement is quite difficult and out of scope. The problem is that we lack information about what is continuous. The relay sees two broadcasts with the same name but different IDs and group numbering. A seamless resume is impossible from the relay's point of view because it's content agnostic; the bytes are either the same or they are different. We purposely don't want the relay to try to interpret the data to decide that these bitstreams are close enough so we could merge them by rewriting timestamps or something. There are a few options to get the desired behavior you want:
2 is the type of thing you would do within a company with a lot of safeguards in place. It's difficult to do it generically, because even something like the |
|
and yeah just to clarify, using the same |
|
Clear: the relay is content-agnostic, so a seamless merge of two independently-numbered broadcasts isn't something it can (or should) infer. It's probably worth clarifying our actual architecture, it reframes your three options for our case. How we run primary distribution. Everything is 1+1 active/active and doubled end-to-end, with the
Most primary IP distribution today has little or no failover at all, so even the bounded behaviour is Your three options mapped to that:
The point we'd underline: none of the three gives no-stall on a hard failure — they each carry a On this PR specifically: the drill validates the shipped #2473 behaviour honestly — a bounded One small question that would help the nice-to-have case for anyone using relay reselect without an |
It used to be lower but I don't remember why I raised it. And yeah, I think what you're saying makes sense. Something at the media layer should support splicing semi-related broadcasts, resuming tracks by rewriting timestamps. It's just difficult to make it generic, maybe if there's an explicit opt-in somewhere in the catalog that |
|
We've got unit tests for all of this behavior now, which is easier to maintain than manual executions. I also had Codex run this test: |
|
Closing this makes sense, and agreed on the principle: unit tests run on every PR, a hand-run drill never does. Three things worth leaving on the record. The numbers line up. On the idle-timeout default. Since it used to be lower, the thing worth recovering is why it went up, before changing it back. The usual reason is a NAT or radio path where a peer with nothing to send gets dropped by an intermediary before the other end notices, so a low timeout buys spurious disconnects on mobile. That trades directly against failover detection, and the two callers want opposite defaults. If it is useful, we can measure the failover side of that trade across a few values on our rig and post the numbers, so any change rests on something more than our single data point. Equally happy to leave it alone. On catalog opt-in splicing. It is worth being explicit about which consumer that feature would serve, because it is probably not us. In primary distribution the two legs are the same content, and the receiver merges them at the packet level (ST 2022-7) precisely because they are identical. Rewriting timestamps to splice two semi-related broadcasts is a different operation, and its output would not be acceptable on a primary feed: a codec or parameter change across the join is a discontinuity the downstream equipment has to reacquire through, which is the visible event we are trying to avoid in the first place. So we would only ever use the strict form of that opt-in, "these are the same content", which is what Following your unit-test point, we opened #2713 as a draft with two model tests drawn from this work. Every existing takeover and reselect test subscribes to a single track, so nothing currently pins that a reselect is decided and served per track. That matters for TS, where one broadcast carries video, audio and data together, and a takeover that restores video while audio quietly stops is worse than a clean failure because it survives monitoring. As a check that the tests are not just restating coverage you already have, we injected a fault that silently drops all but one queued track request: 705 of the 707 existing (Written by Opus 5) |
|
Yeah, if you really want redundancy, you would do active-active. Don't wait for the QUIC idle timeout; always pull both broadcasts and splice them. |
2 participants
The end-to-end drill from #2461, cleaned up for the tree as
just test failover.It is the live counterpart to the routing unit tests in
model/origin.rs: tworeal relays, real publishers whose tracks are created lazily by the demuxer, and
the QUIC idle timeout in the loop.
sub3is load-bearing rather than decorative: it forces relayB to carry thebroadcast via relayA, which is what makes relayA a hop in relayB's own route and
therefore the peer that per-peer announce selection has to treat specially.
Without it the interesting case never arises.
CHECK 1 — failover. Kill
pubA;sub1on relayA must resume, which requiresrelayB to have already advertised the local
pubBstandby to relayA.CHECK 2 — standby join.
sub3on relayB must survivepubBjoining. This isthe case a model-level standby cannot reproduce: it accepts every track request
immediately, whereas
moq importannounces on connect and creates each trackonly once its demuxer reaches it, so a freshly attached standby legitimately
cannot serve some tracks yet.
Results on #2473 (head
cc11cbaf, release, macOS)Both checks pass, repeatably:
sub1resumes 30–33 s after the kill across runs, which is one QUIC idletimeout — so detection dominates and the reselect itself is essentially free.
The standby join is not yet transparent:
sub3stops for 8–9 s beforerecovering at full rate. That is a large improvement on being torn down, but it
is still a visible outage for a viewer of a relay that is only carrying the
broadcast, so the drill measures it and prints a
WARNrather than hiding itbehind a pass. Details in #2473.
Two things about the harness that are easy to get wrong
Both cost me a wrong conclusion, so both are commented in the script.
The timeline is derived, not hard-coded. Killing a publisher sends no
CONNECTION_CLOSE, so the relay keeps serving the dead source until the QUICidle timeout expires and logs nothing at all in between. A grading window shorter
than that budget cannot pass on any build — my original drill killed at t=22 and
graded at t=43, i.e. 21 s into a 30 s timeout, which is how I first reported a
failover that did not exist.
--idlelowers the timeout and the window together.The publisher pipeline is SIGKILLed in one pass. Killing
tspfirst leavesmoq importreading a truncated stream plus EOF, so it shuts its broadcast downcleanly, the relay unannounces immediately, and you grade a graceful detach
instead of a source failure. Worth knowing that the two paths differ sharply: on
the clean-detach path
sub1'sexport tsdies withTS track layout changed after PAT/PMT was emitted: '0.avc3' removedinstead of failing over, even withthe standby announced. I have not chased that one down; noting it in case it is
interesting.
Dependency
Requires
moq --origin, which arrives with #2473, so this should land with orafter it. On a build without the flag the drill exits with a diagnostic rather
than reporting a misleading zero-byte failure. Verified: on
mainit stops atthe preflight; on #2473's head both checks pass.
Not wired into the default
just testaggregate, same asjust test smoke.Needs
ffmpegand TSDuck'stsp; the source clip is generated, with--sourceto publish a real capture instead.
Made with Cursor