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
|
1 participant
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