test(relay): drill bursts across a two-relay cluster on impaired links

[kixelated]

Problem

#4349 reported unanswered FETCHes and 30 s Stream(Old) stalls when a bursty one-frame-per-group control track crossed two cdn.moq.pro nodes. The mock two-relay repro could not produce them because it models neither loss nor flow control. This is the quest/m1/cross-relay-drill.md quest.

Approach

A new drill, bursts_cross_a_cluster, in rs/moq-relay/tests/drills.rs, run in both lanes by lanes!:

• Placement: in drills.rs, so it reuses RelayHost, the seeded moq_shaper paths with verify, and the sensitivity tooling. An origin relay takes the publisher; an edge relay dials it as a cluster peer and takes the subscriber. RelayHost::start now takes a Config (relay_config(port) builds the old default) so the edge can carry cluster.connect.
• Hops shaped: all three (publisher to origin, the peer link, subscriber to edge), each on its own shaper, seeded off one printed base so MOQ_SHAPER_SEED replays all three. The reporter's clients sat 40 ms from their node and the cross-node link is the hop a mock skips, so shaping only one would leave the other's FETCH credit unexercised.
• Pressure: both. Each hop is the drills' profile (20 ms delay, jitter, 5% loss, reorder) behind a 100 kbit/s bottleneck with a 50 ms queue that a 30-group burst overruns. On the impaired lane every endpoint also grants less QUIC credit than a burst: 16 concurrent streams (each group and each FETCH is a stream) and receive windows of a few groups, so MAX_STREAMS/MAX_DATA are waited on over a lossy path. The bottleneck sits below the roughly 160 kbit/s those windows allow, so both bind, and the impaired lane fails unless some burst overflowed a shaper queue (over 13 runs, 7 to 19 datagrams overflowed after connecting, and 10 to 120 per direction queued).
• Recovery model: fetch-on-gap, as the reporter's app does: the default (newest-only) subscription, opened before the first burst, and an immediate FETCH of every unclaimed group below each delivered one. A live group that fails (an Old reset) is FETCHed too. Grading is strict: nothing is evicted and no link drops, so every group has to arrive within 10 s of being written; a lost one fails the drill by name (FETCH failed, FETCH unanswered, live group stalled), and the newest group must come live. Four bursts of 30; the next burst leaves once the previous one starts arriving, so recovery overlaps fresh data.

New mutation fetch-never-sent proves the drill catches FETCHes nobody answers (just test drill-sensitivity fetch-never-sent passes).

Findings

• No unanswered FETCH and no Old stall on main. Over 15 runs every group arrived, the slowest in 2.9 to 3.5 s (116 of 120 via FETCH, since a burst crosses relays newest-first). With 15% loss the slowest was 4.7 s. An occasional live group is reset with Old (a newer group overtook it under max-age 0) and its FETCH recovers it in bounds. A wait-and-reorder subscriber (2 s max age) was also clean.
• Stream credit queues FETCHes. With 8-stream credit, FETCHes waited client-side for credit, a cross-relay round trip per slot, and the tail took about 9 s; every one was answered. That is how a fetch-on-gap app with a 2 s deadline can report a FETCH as failed with nothing lost, but the relay's 10,000-stream default rules that out unless cdn.moq.pro lowers the default.
• Bug fixed: the lite subscriber logged fetch started on every poll while a FETCH waited for stream credit (about 2,000 lines for 116 FETCHes). It now logs once, when the stream opens. No regression test: the only tracing capture helper counts WARN events, and a scripted session that parks open_bi is more harness than the one-line move warrants.

Decisions

Settled with the recommended option while the maintainer was away; each can be revisited.

1. Grading:
   • ✅ Strict: every group must arrive within 10 s, live or by FETCH (recommended)
   • Lenient: a group may fail by name
2. QUIC stream credit on the impaired lane:
   • ✅ 16 concurrent streams (recommended)
   • 8 concurrent streams
3. Wording of the stream-credit finding:
   • ✅ "unless cdn.moq.pro lowers the default" (recommended)
   • "rules it out on cdn.moq.pro"
4. Listing the new drill in rs/moq-relay/AGENTS.md:
   • ✅ Left for the maintainer to approve, since agents do not edit AGENTS.md unprompted (recommended)
   • Add the line in this PR

Impact

• Public API: none.
• Wire: none.
• moq-net: the fetch started info log moves to after the FETCH stream opens.

Alternatives

• A separate two-relay test file: it would duplicate the relay host, shaper, and lanes plumbing.
• Lenient grading (a group may fail by name): a mutation that rejects every FETCH would then pass, and the publisher still holds every group, so a named failure is still a loss.
• 8-stream credit: it makes the drill a FETCH throughput test that needs a 10 s+ bound, not a stall detector.

Follow-ups

• A burst plus a flapping peer link, as a regression for the 0.15.6 route-flap drops (the quest's fourth open question, not taken here; moq-net tests/route_change.rs covers flaps on mocks).
• A moq-transport peer link variant of the drill.
• rs/moq-relay/AGENTS.md still lists three drills; adding bursts_cross_a_cluster there waits on maintainer approval (decision 4).
• quest/m1/cross-relay-bursts.md now records the drill's result and still waits on the reporter's re-run.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 70f06e5f-a4a3-4a22-9270-737eef675874

📥 Commits

Reviewing files that changed from the base of the PR and between 8714109 and aaba76b.

📒 Files selected for processing (7)

• quest/m1/README.md
• quest/m1/cross-relay-bursts.md
• quest/m1/cross-relay-drill.md
• rs/moq-net/src/lite/subscriber.rs
• rs/moq-relay/tests/drills.rs
• test/drill/README.md
• test/drill/mutations/fetch-never-sent.patch

💤 Files with no reviewable changes (2)

• quest/m1/README.md
• quest/m1/cross-relay-drill.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

---

Walkthrough

The test suite adds a two-relay burst drill that checks live delivery and FETCH recovery under loopback and impaired network conditions. The harness adds seeded path shaping and configurable relay and client setup. Documentation and sensitivity coverage describe the drill. The quest documents record observed results and remove the former planned drill. FETCH start logging now occurs after stream opening succeeds.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to aaba7

The new drill and its sensitivity patch have no identified merge-blocking issue. Merge after normal checks.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the new two-relay burst drill and its impaired-link focus.
Description check	✅ Passed	The description explains the problem, approach, findings, and scope of the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 86.96% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 23 functions across 2 files. (3 skipped: 3 …
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

✨ Simplify code

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[kixelated]

Automated review: #4920 at 6ca1afef

A useful drill. It puts the #4349 shape (bursts of one-frame groups, fetch-on-gap, two relays) through real QUIC on all three hops, grades each group strictly and by name, and the fetch-never-sent mutation shows it catches FETCHes nobody answers. Moving the fetch started log after Stream::poll_open (lite/subscriber.rs:4469-4471) is correct: the open re-polls this state while it waits for stream credit, and the GOAWAY and open-error paths return before the log either way. The RelayHost::start(Config) refactor and the moved port assert keep the old behavior. Cleanup is fine: RelayHost drops its runtime, and a panic drops the JoinSet, which aborts its tasks.

Findings

1. Medium: on the impaired lane, the 1 Mbit/s bottleneck probably never fills, and nothing checks whether it does.
credit() (drills.rs:753-757) gives every endpoint a 1 KiB connection receive window (4 * PAYLOAD). Each hop's RTT is about 40-50 ms (20 ms ±5 ms each way). So no sender can have more than about 1 KiB outstanding per RTT, roughly 20 KB/s (about 170-200 kbit/s), well under the 1 Mbit/s bucket in bursty() (drills.rs:738). The bucket's 50 ms queue holds about 6 KB at 1 Mbit/s, and stream data can't arrive fast enough to fill it. So the "burst overruns a bottleneck" pressure (bursty() docs, the drill's doc comment, test/drill/README.md, and the finding written into quest/m1/cross-relay-bursts.md) is most likely not happening. The loopback lane has no shaper, so neither lane tests it. Shaper::verify won't catch this: it only checks loss, reorder, and delay (IMPAIRMENTS in moq-shaper/src/lib.rs:232), not throttled or overflowed.
Fix: check the printed impaired: ... overflowed counters. If they are 0, pick one:

• assert overflowed > 0 on at least the peer or subscriber shaper (Path::verify could return the Stats);
• raise the receive windows so the bucket is the limit that binds;
• run credit pressure and the bottleneck as separate variants;
• or drop the bottleneck claim from the docs and the quest finding.

2. Low: a failing impaired run can't be replayed from its seeds.
Each of the three Path::shaped calls draws its own rand::random() seed and prints it (drills.rs:115-128). MOQ_SHAPER_SEED gives one seed to all three shapers. So setting any printed value runs three identically seeded paths, which is not the run that failed. The comment at :113-114 promises replay. Fix: draw one base seed per drill, derive one per path (for example base.wrapping_add(index)), and print the base.

3. Nit: the loop's outer timeout equals the per-task timeout.
The step timeout (drills.rs:907) and each settle task's timeout (:893, plus the live read) are both SETTLE. If the last batch of FETCHes is spawned in the same timer tick as the step that starts after it, the generic stalled: nothing arrived panic can fire before the named Unanswered/Stalled outcome. The test still fails, but the label can differ from the # expect: Unanswered in fetch-never-sent.patch. Batches are milliseconds apart in practice, so this is unlikely. A margin (SETTLE + Duration::from_secs(1)) removes it.

CI: Android, Quest, WASM, and Windows pass. Check, Test, and macOS (the jobs that run the drill) are still pending.

Verdict: MERGE once CI green, after checking the shaper counters for finding 1 and either fixing the setup or rewording the docs and quest finding. Reviewed head 6ca1afef853459feb1351c2cfd6516cc9e87319a.

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Addressed the automated review of 6ca1afef in c482cb9d:

1. Confirmed: every shaper reported 0 overflowed at 1 Mbit/s, since the 1 KiB connection window caps a sender near 160 kbit/s. The bottleneck is now 100 kbit/s, below that cap, so both pressures bind, and Path::verify returns the shaper's Stats so the impaired lane asserts some burst overflowed a queue. Over 15 runs: 19 to 48 overflowed datagrams per run, slowest group 2.9 to 3.5 s. The docs and the quest finding now say 100 kbit/s.
2. Fixed: the drill draws one base seed, prints it as MOQ_SHAPER_SEED=..., and seeds its three paths base, base + 1, base + 2, so the printed value replays the run. Single-path drills are unchanged.
3. Fixed: the step timeout is SETTLE + 1s, so a group that times out fails as Unanswered or Stalled before the generic stall fires.

(Written by Claude Opus 5.5)

[kixelated]

Automated follow-up review: #4920 at c482cb9d

Re-review after the push of c482cb9d ("make the cluster drill's bottleneck overflow and replayable"). It addresses all three earlier findings:

• 1 (bottleneck never fills): mostly fixed. The bucket in bursty() is now 100 kbit/s (drills.rs:757), below the roughly 160 kbit/s the 1 KiB windows allow, and the impaired lane now fails unless something overflowed (drills.rs:1031-1038). The README and quest finding are updated to match. Two caveats follow below.
• 2 (seeds can't replay a run): fixed. seed() draws one base and prints it as MOQ_SHAPER_SEED=, and the three paths get seed, +1, and +2 (drills.rs:832-847). The other drills each start one path, so they're unaffected.
• 3 (outer timeout equals the per-task timeout): fixed. The step timeout is now SETTLE + 1s.

Findings

A. Low: the new overflowed > 0 check may be satisfied by the QUIC handshake, not by a burst.
The check sums overflow over the whole run (drills.rs:1031-1035), and at this rate nearly any two full-size datagrams sent back to back overflow (see B). The server's first flight is a padded 1200-byte Initial datagram, often followed by more Handshake datagrams, so a handshake can bump overflowed before any group is published. With the 1 KiB connection windows, a sender mostly has about one data packet in flight per round trip, so it isn't clear the bursts themselves ever overflow. The assert message ("no burst overran a bottleneck") could then pass without a burst causing it.
Fix: take shaper.stats() on each path right before the first burst and assert on the change in overflowed after the drill, rather than on the total. Or print both values once in CI to check.

B. Low: the 50 ms queue is shorter than it takes one full packet to leave at 100 kbit/s, so the bottleneck drops packets rather than queuing them.
At 100 kbit/s, a 1200-byte datagram takes about 96 ms to leave (Rate::cost, moq-shaper/src/lib.rs:307), and the 1500-byte burst allowance covers about 120 ms. Following treat() (lib.rs:900-916), the first full-size datagram of a burst goes straight through. The next one, sent right behind it, would have to wait about 72 ms, which is more than the 50 ms queue, so it's dropped. That repeats until the bucket drains for about 22 ms. Only small packets like ACKs ever wait in the queue. That's still pressure, but it's mostly tail drops, while the bursty() doc and README say "its tail waits in the queue or is dropped from it". If you want real queuing delay as well, raise queue to about 250 ms, which holds a couple of full packets. Otherwise, say in the docs that the bottleneck mostly drops.

CI: everything except the skipped Auto-merge is still pending on this head (Check, Test, macOS, Android, WASM, Windows, Quest). The PR is MERGEABLE.

Verdict: MERGE once CI green. A and B are about whether the test measures what its docs say, not about the relay itself. Reviewed head c482cb9d956b4f372e1d31d9edfc0d9cdb8e986e.

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Follow-up review of c482cb9d, addressed in aaba76b3:

• A (handshake could satisfy the check): fixed. Each path's overflow is now snapshotted once connected and subscribed, and the impaired lane asserts on the overflow during the bursts. Measured that way the bursts do overflow on their own: 7 to 19 datagrams per run over 13 runs (the earlier totals of 19 to 48 included setup).
• B (50 ms queue only drops): declined, measured. The analysis assumes 1200-byte datagrams, but the 1 KiB connection window keeps burst datagrams small, so the 50 ms queue holds one or two: 10 to 120 datagrams per direction were throttled (queued) per run, alongside the drops. Raising the queue went the other way: at 100 ms the bursts overflowed in 1 of 3 runs, and at 150 ms and 250 ms in none, so the check could not hold. The bursty() doc now says the queue holds one or two small datagrams before it drops the rest.

(Written by Claude Opus 5.5)

[kixelated]

Automated follow-up review: #4920 at aaba76b3

Re-review after the push of aaba76b3 ("grade only the cluster drill's burst overflow"). It's a test-only change to rs/moq-relay/tests/drills.rs and it addresses both findings from the c482cb9d review:

• A (handshake could satisfy the overflow check): fixed. setup_overflowed is snapshotted after subscribe() returns (drills.rs:893-895), and the impaired assert now checks the change since then (:1007-1017). subscribe() parks on routed_broadcast, which can only resolve once the edge's cluster link to the origin and the subscriber's session are both carrying announcements, so all three handshakes (peer, publish, subscribe) finish before the snapshot. The counters only go up, so the subtraction can't underflow. The fault-activation line now also prints the burst-only count, which makes the lane easy to inspect in CI logs.
• B (50 ms queue mostly drops): addressed by keeping the queue and correcting the docs. The bursty() doc now says the window keeps burst datagrams small, so the queue holds one or two before it drops. That's consistent with the GCRA math: with 256-byte groups and a 1 KiB connection window, burst datagrams are a few hundred bytes, and the 50 ms queue holds about 625 bytes at 100 kbit/s. The commit message's note that 100 ms and longer queues absorbed every burst, so the check would never fire, is a good reason to keep 50 ms.

Path::verify no longer returns Stats. It still fails the drill if the profile did nothing, and each path is still verified after the sessions drop. The paths array of borrows is last used before those moves, so ownership is fine.

Findings

No new issues.

CI: on this head Check, Test, macOS, Android, WASM, and Windows are still pending, and Auto-merge is skipped. On c482cb9d, Check, Test, macOS, Android, and Windows passed. The PR is MERGEABLE.

Verdict: MERGE once CI green. Reviewed head aaba76b3289e665804736d3b2c62097adc3542bd.

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit aaba76b.

No new actionable correctness finding after full diff, supporting helpers and prior review discussion. Direction is strong: reuse the real-QUIC drill harness, exercise three independently shaped hops, require every group rather than allowing named losses, and include an unanswered-FETCH mutation.

rs/moq-relay/tests/drills.rs:893-895,1007-1017 subtracts setup overflow, so handshake drops alone cannot satisfy fault activation; :919-999 separates live and FETCH outcomes and retries failed live reads; :1019-1036 checks loss, write-to-observation latency and newest-live delivery. The 11s outer step wait lets the 10s per-read/FETCH timeout report a named outcome. The log move in rs/moq-net/src/lite/subscriber.rs:4469-4472 is after successful poll_open and before leaving Open, eliminating repeated logs while stream credit is pending. Shared seed plus wrapping offsets is simpler and reproducible within the documented scheduling limits.

Limits: static review only; no independent real-QUIC, sensitivity-mutation or seeded stress execution. Earlier queue/overflow comments are addressed in the latest code and documented measurements, not repeated as inline findings. Current head has Android/Platform success, Check queued and WASM pending; GitHub reports mergeable=false, so resolve conflicts and rerun the affected drills on the merge result before landing. Head/state/reviews rechecked.

[kixelated]

The maintainer selected merge for this PR. Final head aaba76b3289e665804736d3b2c62097adc3542bd has a Codex review with no actionable finding and green required CI. The earlier overflow and seed findings are addressed.

Independent Nix validation passed both bursts_cross_a_cluster lanes and quest check. The fetch-never-sent sensitivity proof passed: the baseline delivers, and removing FETCH transmission fails with Unanswered.

The change adds two-relay impaired-link coverage and moves FETCH logging after stream opening. Public API and wire impact: none. The flapping-peer follow-up is proposed in #4930; the reporter re-run remains a separate quest. No agent-guidance changes are included.

(Written by GPT-6)