From 165377f8e8abebf8f6a1e7244180685ab8011723 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 22:05:17 -0700 Subject: [PATCH 01/19] chore: claim shaper profiles quest From 1c47bf8f11b27223f36678162e0ca779bf2f3d7d Mon Sep 17 00:00:00 2001 From: fperex <270788582+fperex@users.noreply.github.com> Date: Fri, 25 Sep 2026 06:57:25 -0400 Subject: [PATCH 02/19] feat(shaper): pipe TCP on the shaper's port to the target, as an option A relay serves HTTP on the port number it serves QUIC on, and a browser fetches /certificate.sha256 from it before it dials WebTransport. With only a UDP socket on the shaper's port that fetch fails, so a browser cannot be put behind the shaper at all. `Setup` is a `Config` plus the options it has no field for, and `Shaper::bind` takes anything that converts into one, so every existing caller passing a `Config` compiles and behaves as before. Its first option, `tcp_passthrough`, binds a TCP listener on the port the UDP socket got and copies each connection to the target untouched. TCP is not impaired: a reliable transport cannot shed load, so shaping it would measure TCP's retransmission instead of the profile. The CLI takes it as `--tcp-passthrough`. Off by default. Co-Authored-By: Claude Opus 5.5 --- rs/moq-shaper/Cargo.toml | 2 +- rs/moq-shaper/src/lib.rs | 128 ++++++++++++++++++++++++++++++++++++-- rs/moq-shaper/src/main.rs | 9 ++- 3 files changed, 133 insertions(+), 6 deletions(-) diff --git a/rs/moq-shaper/Cargo.toml b/rs/moq-shaper/Cargo.toml index a923d0f2f1..ea617a976d 100644 --- a/rs/moq-shaper/Cargo.toml +++ b/rs/moq-shaper/Cargo.toml @@ -21,4 +21,4 @@ anyhow = { workspace = true } clap = { workspace = true } humantime = { workspace = true } rand = { workspace = true } -tokio = { workspace = true, features = ["macros", "net", "rt-multi-thread", "signal", "sync", "time"] } +tokio = { workspace = true, features = ["io-util", "macros", "net", "rt-multi-thread", "signal", "sync", "time"] } diff --git a/rs/moq-shaper/src/lib.rs b/rs/moq-shaper/src/lib.rs index 730d3a9814..c741f1cb6e 100644 --- a/rs/moq-shaper/src/lib.rs +++ b/rs/moq-shaper/src/lib.rs @@ -28,7 +28,12 @@ use std::{ use anyhow::Context; use rand::{RngExt, SeedableRng, rngs::Xoshiro256PlusPlus}; -use tokio::{net::UdpSocket, sync::mpsc, task::JoinSet, time::Instant}; +use tokio::{ + net::{TcpListener, TcpStream, UdpSocket}, + sync::mpsc, + task::JoinSet, + time::Instant, +}; /// How one direction of the path treats each datagram. /// @@ -146,6 +151,32 @@ pub struct Config { pub down: Profile, } +/// A [`Config`] plus the opt-in options it has no field for. +/// +/// Every option defaults to off, so a setup made from a config shapes exactly +/// as that config does. +#[derive(Clone, Debug)] +pub struct Setup { + /// Where to listen and forward, the seed, and each direction's profile. + pub config: Config, + /// Also accept TCP on the listening port and pipe it to the target untouched. + /// + /// A relay serves HTTP on the port number it serves QUIC on, and a browser + /// fetches the certificate hash from it before it dials WebTransport. TCP is + /// never impaired: a reliable transport cannot shed load, so shaping it + /// would measure how TCP retransmits. + pub tcp_passthrough: bool, +} + +impl From for Setup { + fn from(config: Config) -> Self { + Self { + config, + tcp_passthrough: false, + } + } +} + /// What one direction did, summed over every client. #[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] pub struct Counters { @@ -200,7 +231,9 @@ pub struct Shaper { impl Shaper { /// Bind the listening socket and start forwarding. - pub async fn bind(config: Config) -> anyhow::Result { + pub async fn bind(setup: impl Into) -> anyhow::Result { + let setup = setup.into(); + let config = setup.config; config.up.validate().context("invalid up profile")?; config.down.validate().context("invalid down profile")?; @@ -209,6 +242,16 @@ impl Shaper { .with_context(|| format!("bind {}", config.bind))?; let addr = listen.local_addr()?; + // The port the UDP socket got, since a relay serves HTTP on its QUIC port. + let tcp = match setup.tcp_passthrough { + true => Some( + TcpListener::bind(addr) + .await + .with_context(|| format!("bind the TCP passthrough on {addr}"))?, + ), + false => None, + }; + let tally = Arc::new([Tally::default(), Tally::default()]); let failed = Arc::new(OnceLock::new()); let task = tokio::spawn({ @@ -216,7 +259,7 @@ impl Shaper { let tally = tally.clone(); let failed = failed.clone(); async move { - if let Err(err) = run(Arc::new(listen), config, tally).await { + if let Err(err) = run(Arc::new(listen), tcp, config, tally).await { let _ = failed.set(format!("{err:#}")); } } @@ -310,7 +353,12 @@ fn bump(counter: &AtomicU64) { /// /// A flow is a socket of its own toward the target, so the target sees one /// address per client just as it would without the shaper in the way. -async fn run(listen: Arc, config: Config, tally: Arc<[Tally; 2]>) -> anyhow::Result<()> { +async fn run( + listen: Arc, + tcp: Option, + config: Config, + tally: Arc<[Tally; 2]>, +) -> anyhow::Result<()> { let mut flows = HashMap::::new(); let mut tasks = JoinSet::new(); let mut buf = vec![0u8; u16::MAX as usize]; @@ -322,6 +370,11 @@ async fn run(listen: Arc, config: Config, tally: Arc<[Tally; 2]>) -> res.context("flow task panicked")??; continue; } + res = accept(tcp.as_ref()) => { + let (stream, _) = res.context("accept a TCP connection")?; + tasks.spawn(pipe(stream, config.target)); + continue; + } }; let now = Instant::now(); @@ -347,6 +400,23 @@ async fn run(listen: Arc, config: Config, tally: Arc<[Tally; 2]>) -> } } +/// The next TCP connection, or never without a passthrough. +async fn accept(tcp: Option<&TcpListener>) -> std::io::Result<(TcpStream, SocketAddr)> { + match tcp { + Some(listener) => listener.accept().await, + None => std::future::pending().await, + } +} + +/// Copy one TCP connection to the target and back, untouched. +async fn pipe(mut client: TcpStream, target: SocketAddr) -> anyhow::Result<()> { + // Either end refusing or hanging up ends that connection, not the shaper. + if let Ok(mut server) = TcpStream::connect(target).await { + let _ = tokio::io::copy_bidirectional(&mut client, &mut server).await; + } + Ok(()) +} + /// A socket that can reach `target`: loopback for a loopback target, so the /// shaper never opens a port beyond the host when it does not need to. async fn bind_toward(target: SocketAddr) -> anyhow::Result { @@ -724,4 +794,54 @@ mod tests { }; refused(undelayed, "needs a delay").await; } + + #[tokio::test] + async fn tcp_passes_through_untouched() { + use tokio::io::{AsyncReadExt, AsyncWriteExt}; + + // A relay answering HTTP on its QUIC port, reduced to an echo. + let server = TcpListener::bind(LOCALHOST).await.unwrap(); + let target = server.local_addr().unwrap(); + tokio::spawn(async move { + let (mut stream, _) = server.accept().await.unwrap(); + let mut buf = [0u8; 64]; + let size = stream.read(&mut buf).await.unwrap(); + stream.write_all(&buf[..size]).await.unwrap(); + }); + + // A profile that would lose every datagram, to show TCP skips it. + let blackhole = Profile { + loss: 1.0, + ..Default::default() + }; + let config = Config { + bind: LOCALHOST, + target, + seed: 13, + up: blackhole.clone(), + down: blackhole, + }; + let shaper = Shaper::bind(Setup { + tcp_passthrough: true, + ..config.into() + }) + .await + .unwrap(); + + let mut client = TcpStream::connect(shaper.addr()).await.unwrap(); + client.write_all(b"/certificate.sha256").await.unwrap(); + let mut buf = [0u8; 64]; + let size = tokio::time::timeout(Duration::from_secs(2), client.read(&mut buf)) + .await + .unwrap() + .unwrap(); + assert_eq!(&buf[..size], b"/certificate.sha256"); + assert_eq!(shaper.stats(), Stats::default(), "TCP reached the datagram path"); + } + + #[tokio::test] + async fn tcp_is_refused_without_the_passthrough() { + let (shaper, _client) = setup(1, Profile::default(), Profile::default()).await; + assert!(TcpStream::connect(shaper.addr()).await.is_err()); + } } diff --git a/rs/moq-shaper/src/main.rs b/rs/moq-shaper/src/main.rs index 2a2841e101..cabb72f9ff 100644 --- a/rs/moq-shaper/src/main.rs +++ b/rs/moq-shaper/src/main.rs @@ -40,6 +40,9 @@ struct Args { /// The longest a datagram waits for the rate limit before it is dropped. #[arg(long, default_value = "100ms", value_parser = humantime::parse_duration, requires = "rate")] queue: Duration, + /// Also pipe TCP on the listening port to the target, untouched. + #[arg(long)] + tcp_passthrough: bool, } #[tokio::main] @@ -57,12 +60,16 @@ async fn main() -> anyhow::Result<()> { queue: args.queue, }), }; - let shaper = moq_shaper::Shaper::bind(moq_shaper::Config { + let config = moq_shaper::Config { bind: args.listen, target: args.target, seed: args.seed.unwrap_or_else(rand::random), up: profile.clone(), down: profile, + }; + let shaper = moq_shaper::Shaper::bind(moq_shaper::Setup { + tcp_passthrough: args.tcp_passthrough, + ..config.into() }) .await?; From a909d5faf108b401b9c86cafb3ff6574a0b1ff6f Mon Sep 17 00:00:00 2001 From: fperex <270788582+fperex@users.noreply.github.com> Date: Fri, 25 Sep 2026 07:01:19 -0400 Subject: [PATCH 03/19] feat(shaper): a gaussian jitter model that keeps the order, as an option The uniform jitter draws each datagram's delay on its own, so a later datagram can overtake an earlier one. A drill wants that, but QUIC reads the overtake as loss: it retransmits and backs off, and a run meant to measure a jittery path measures the congestion response instead. On a 5ms delay with 5ms of jitter, that alone dragged a receiver's audio buffer from 120ms to nearly two seconds. `Options::jitter_model` picks how `Profile::jitter` is drawn, per direction, through `Setup::up` and `Setup::down`. `Jitter::Uniform` stays the default and is unchanged, draws included. `Jitter::Gaussian` treats `jitter` as a sigma, clamps the draw at zero, and never lets a datagram leave before the one in front: queueing delay on a FIFO path, which varies the spacing and never the order. Only `reorder` still overtakes. Its sigma may exceed the delay, which the uniform model refuses. Box-Muller takes exactly two draws per datagram, so the other knobs' streams stay aligned. It counts `delayed` like the uniform model whenever a delay is configured, so `verify` holds it to the same chance. The CLI takes `--jitter-model gaussian`. Co-Authored-By: Claude Opus 5.5 --- rs/moq-shaper/src/lib.rs | 294 +++++++++++++++++++++++++++++++++++--- rs/moq-shaper/src/main.rs | 24 +++- 2 files changed, 299 insertions(+), 19 deletions(-) diff --git a/rs/moq-shaper/src/lib.rs b/rs/moq-shaper/src/lib.rs index c741f1cb6e..c9d54744ec 100644 --- a/rs/moq-shaper/src/lib.rs +++ b/rs/moq-shaper/src/lib.rs @@ -38,13 +38,14 @@ use tokio::{ /// How one direction of the path treats each datagram. /// /// A datagram is first subject to `loss`, then waits for the `rate` limit, then -/// takes `delay` plus or minus `jitter` to arrive, unless `reorder` sends it -/// ahead of everything still in flight. +/// takes `delay` plus or minus `jitter`, drawn as its [`Jitter`] model says, to +/// arrive, unless `reorder` sends it ahead of everything still in flight. #[derive(Clone, Debug, Default, PartialEq)] pub struct Profile { /// The base one-way delay. pub delay: Duration, - /// The most a datagram's delay varies from `delay`, uniformly either way. + /// How far a datagram's delay varies from `delay`: the most either way when + /// [`Jitter::Uniform`], the sigma when [`Jitter::Gaussian`]. pub jitter: Duration, /// The probability that a datagram is dropped. pub loss: f64, @@ -55,7 +56,7 @@ pub struct Profile { } impl Profile { - fn validate(&self) -> anyhow::Result<()> { + fn validate(&self, model: Jitter) -> anyhow::Result<()> { anyhow::ensure!( (0.0..=1.0).contains(&self.loss), "loss {} is not a probability", @@ -71,8 +72,9 @@ impl Profile { "reorder {} needs a delay to overtake", self.reorder ); + // A gaussian clamps its draw at zero instead, as a queue cannot run early. anyhow::ensure!( - self.jitter <= self.delay, + model == Jitter::Gaussian || self.jitter <= self.delay, "jitter {:?} exceeds delay {:?}, which would need a negative delay", self.jitter, self.delay @@ -84,6 +86,36 @@ impl Profile { } } +/// How a direction draws each datagram's jitter. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub enum Jitter { + /// Uniform within `jitter` either way of `delay`, drawn for each datagram on + /// its own, so a later datagram can overtake an earlier one. + #[default] + Uniform, + /// A gaussian with `jitter` as its sigma, clamped at zero, that never leaves + /// before the datagram in front: it varies the spacing, never the order. + /// + /// This is queueing delay on a FIFO path. A jitter that overtakes hands QUIC + /// a gap it can only read as loss, so the run measures its congestion + /// response rather than the jitter. + Gaussian, +} + +/// One direction's opt-in options beyond its [`Profile`]. The default adds nothing. +#[derive(Clone, Debug, Default, PartialEq)] +pub struct Options { + /// How the profile's `jitter` is drawn. + pub jitter_model: Jitter, +} + +impl Options { + /// Check the options, and `profile` as they draw it. + fn validate(&self, profile: &Profile) -> anyhow::Result<()> { + profile.validate(self.jitter_model) + } +} + /// Each impairment a profile draws for: its name, how likely it is to act on /// any one datagram (zero when unconfigured), and the count showing it did. /// @@ -166,6 +198,10 @@ pub struct Setup { /// never impaired: a reliable transport cannot shed load, so shaping it /// would measure how TCP retransmits. pub tcp_passthrough: bool, + /// The options from a client toward the target. + pub up: Options, + /// The options from the target back toward a client. + pub down: Options, } impl From for Setup { @@ -173,6 +209,8 @@ impl From for Setup { Self { config, tcp_passthrough: false, + up: Options::default(), + down: Options::default(), } } } @@ -222,7 +260,7 @@ impl fmt::Display for Stats { /// A running shaper. Dropping it stops forwarding. pub struct Shaper { addr: SocketAddr, - config: Config, + setup: Setup, tally: Arc<[Tally; 2]>, /// Why forwarding stopped, if it did. failed: Arc>, @@ -233,9 +271,9 @@ impl Shaper { /// Bind the listening socket and start forwarding. pub async fn bind(setup: impl Into) -> anyhow::Result { let setup = setup.into(); - let config = setup.config; - config.up.validate().context("invalid up profile")?; - config.down.validate().context("invalid down profile")?; + let config = &setup.config; + setup.up.validate(&config.up).context("invalid up profile")?; + setup.down.validate(&config.down).context("invalid down profile")?; let listen = UdpSocket::bind(config.bind) .await @@ -255,11 +293,11 @@ impl Shaper { let tally = Arc::new([Tally::default(), Tally::default()]); let failed = Arc::new(OnceLock::new()); let task = tokio::spawn({ - let config = config.clone(); + let setup = setup.clone(); let tally = tally.clone(); let failed = failed.clone(); async move { - if let Err(err) = run(Arc::new(listen), tcp, config, tally).await { + if let Err(err) = run(Arc::new(listen), tcp, setup, tally).await { let _ = failed.set(format!("{err:#}")); } } @@ -267,7 +305,7 @@ impl Shaper { Ok(Self { addr, - config, + setup, tally, failed, task, @@ -281,7 +319,7 @@ impl Shaper { /// The configuration this shaper runs, including its seed. pub fn config(&self) -> &Config { - &self.config + &self.setup.config } /// What the shaper has done so far. @@ -303,7 +341,7 @@ impl Shaper { anyhow::bail!("the shaper stopped forwarding: {err} ({stats})"); } - let missing = unapplied(&self.config, &stats); + let missing = unapplied(&self.setup.config, &stats); anyhow::ensure!( missing.is_empty(), "the profile never applied {} ({stats}), so this run was not impaired as configured", @@ -356,9 +394,10 @@ fn bump(counter: &AtomicU64) { async fn run( listen: Arc, tcp: Option, - config: Config, + setup: Setup, tally: Arc<[Tally; 2]>, ) -> anyhow::Result<()> { + let config = &setup.config; let mut flows = HashMap::::new(); let mut tasks = JoinSet::new(); let mut buf = vec![0u8; u16::MAX as usize]; @@ -390,10 +429,10 @@ async fn run( let (down_tx, down_rx) = mpsc::unbounded_channel(); tasks.spawn(deliver(down_rx, listen.clone(), from)); - let down = Link::new(&config.down, config.seed, stream + 1, now, down_tx); + let down = Link::new(&config.down, &setup.down, config.seed, stream + 1, now, down_tx); tasks.spawn(reply(upstream, config.target, down, tally.clone())); - entry.insert(Link::new(&config.up, config.seed, stream, now, up_tx)) + entry.insert(Link::new(&config.up, &setup.up, config.seed, stream, now, up_tx)) } }; link.push(now, buf[..size].to_vec(), &tally[UP]); @@ -491,9 +530,12 @@ async fn deliver( /// One direction of one flow: the decisions and the state they depend on. struct Link { profile: Profile, + jitter: Jitter, rng: Xoshiro256PlusPlus, /// When the token bucket would be full again, as in GCRA. full_at: Instant, + /// When the latest in-order datagram leaves, which the next one never beats. + floor: Instant, queue: mpsc::UnboundedSender<(Instant, Vec)>, } @@ -504,6 +546,7 @@ impl Link { /// client's ephemeral port is no identity across runs. fn new( profile: &Profile, + options: &Options, seed: u64, stream: u64, now: Instant, @@ -511,10 +554,12 @@ impl Link { ) -> Self { Self { profile: profile.clone(), + jitter: options.jitter_model, rng: Xoshiro256PlusPlus::seed_from_u64(seed ^ stream.wrapping_mul(0x9E37_79B9_7F4A_7C15)), // Not the moment the link is built, which is after `now`: that would // make the first datagram queue behind a bucket still refilling. full_at: now, + floor: now, queue, } } @@ -526,7 +571,10 @@ impl Link { // knob's outcome never shifts the stream another knob draws from. let lose = self.rng.random::() < self.profile.loss; let skip = self.rng.random::() < self.profile.reorder; - let spread = self.rng.random::() * 2.0 - 1.0; + let spread = match self.jitter { + Jitter::Uniform => self.rng.random::() * 2.0 - 1.0, + Jitter::Gaussian => gaussian(&mut self.rng), + }; if lose { bump(&tally.lost); @@ -554,6 +602,17 @@ impl Link { if skip { bump(&tally.reordered); + } else if self.jitter == Jitter::Gaussian { + // A queue: nothing leaves before the datagram in front of it. + let wait = self.profile.delay.as_secs_f64() + self.profile.jitter.as_secs_f64() * spread; + let leave = (depart + Duration::from_secs_f64(wait.max(0.0))).max(self.floor); + // Counted like the uniform model whenever a delay is configured, so + // the chance `verify` holds it to is the same. + if !self.profile.delay.is_zero() || leave > depart { + bump(&tally.delayed); + } + self.floor = leave; + depart = leave; } else if !self.profile.delay.is_zero() { let jitter = self.profile.jitter.as_secs_f64() * spread; depart += Duration::from_secs_f64(self.profile.delay.as_secs_f64() + jitter); @@ -565,6 +624,14 @@ impl Link { } } +/// A standard normal sample by Box-Muller, which always takes exactly two draws, +/// so the jitter never shifts the stream the next datagram's knobs draw from. +fn gaussian(rng: &mut Xoshiro256PlusPlus) -> f64 { + let u1 = rng.random::().max(f64::MIN_POSITIVE); + let u2 = rng.random::(); + (-2.0 * u1.ln()).sqrt() * (std::f64::consts::TAU * u2).cos() +} + #[cfg(test)] mod tests { use super::*; @@ -844,4 +911,195 @@ mod tests { let (shaper, _client) = setup(1, Profile::default(), Profile::default()).await; assert!(TcpStream::connect(shaper.addr()).await.is_err()); } + + /// An echo server, a shaper `setup` builds from a config aimed at it, and a + /// client dialing the shaper. + async fn shaped(setup: impl FnOnce(Config) -> Setup) -> (Shaper, UdpSocket) { + let echo = UdpSocket::bind(LOCALHOST).await.unwrap(); + let target = echo.local_addr().unwrap(); + tokio::spawn(async move { + let mut buf = vec![0u8; u16::MAX as usize]; + loop { + let (size, from) = echo.recv_from(&mut buf).await.unwrap(); + echo.send_to(&buf[..size], from).await.unwrap(); + } + }); + + let config = Config { + bind: LOCALHOST, + target, + seed: 3, + up: Profile::default(), + down: Profile::default(), + }; + let shaper = Shaper::bind(setup(config)).await.unwrap(); + + let client = UdpSocket::bind(LOCALHOST).await.unwrap(); + client.connect(shaper.addr()).await.unwrap(); + (shaper, client) + } + + /// Push `count` numbered datagrams through one link, `spacing` apart, and + /// return each id with its departure, in the order they would be sent. + fn departures( + profile: &Profile, + options: &Options, + count: u32, + spacing: Duration, + ) -> (Vec<(u32, Instant)>, Counters) { + let (queue, mut queued) = mpsc::unbounded_channel(); + let now = Instant::now(); + let mut link = Link::new(profile, options, 7, 0, now, queue); + let tally = Tally::default(); + for id in 0..count { + link.push(now + spacing * id, id.to_be_bytes().to_vec(), &tally); + } + + let mut sent = Vec::new(); + while let Ok((at, datagram)) = queued.try_recv() { + sent.push((u32::from_be_bytes(datagram[..4].try_into().unwrap()), at)); + } + // Ties leave in arrival order, which is id order here. + sent.sort_by_key(|&(id, at)| (at, id)); + (sent, tally.snapshot()) + } + + const GAUSSIAN: Options = Options { + jitter_model: Jitter::Gaussian, + }; + + #[tokio::test] + async fn only_the_gaussian_model_keeps_the_order() { + let jittery = Profile { + delay: Duration::from_millis(20), + jitter: Duration::from_millis(10), + ..Default::default() + }; + + let (_uniform, client) = shaped(|config| { + Config { + up: jittery.clone(), + ..config + } + .into() + }) + .await; + let got = round_trip(&client, 100).await; + let mut sorted = got.clone(); + sorted.sort(); + assert_eq!(sorted, (0..100).collect::>(), "jitter lost datagrams"); + assert_ne!(got, sorted, "uniform jitter never overtook, so this proves nothing"); + + let (gaussian, client) = shaped(|config| Setup { + up: GAUSSIAN, + ..Config { up: jittery, ..config }.into() + }) + .await; + let got = round_trip(&client, 100).await; + assert_eq!(got, (0..100).collect::>(), "gaussian jitter reordered"); + + let stats = gaussian.verify().unwrap(); + assert_eq!(stats.up.reordered, 0, "{stats}"); + assert_eq!(stats.up.delayed, 100, "{stats}"); + } + + #[test] + fn gaussian_jitter_never_leaves_before_it_arrived() { + // A sigma far past the delay, so most draws would be negative unclamped. + let wide = Profile { + jitter: Duration::from_millis(50), + ..Default::default() + }; + let now = Instant::now(); + let (sent, counters) = departures(&wide, &GAUSSIAN, 1000, Duration::ZERO); + assert_eq!(sent.len(), 1000); + assert!(sent.iter().all(|&(_, at)| at >= now)); + assert!(counters.delayed > 0, "{counters}"); + } + + #[test] + fn gaussian_jitter_alone_never_changes_the_order() { + // Ten times the arrival spacing as sigma, so an independent draw per + // datagram would shuffle nearly all of them. + let jittery = Profile { + delay: Duration::from_millis(5), + jitter: Duration::from_millis(50), + ..Default::default() + }; + let (sent, counters) = departures(&jittery, &GAUSSIAN, 2000, Duration::from_millis(5)); + let ids: Vec = sent.iter().map(|&(id, _)| id).collect(); + assert_eq!(ids, (0..2000).collect::>(), "gaussian jitter reordered"); + assert_eq!(counters.reordered, 0); + } + + #[test] + fn gaussian_jitter_still_varies_the_spacing() { + let spacing = Duration::from_millis(20); + let jittery = Profile { + delay: Duration::from_millis(5), + jitter: Duration::from_millis(5), + ..Default::default() + }; + let (sent, _) = departures(&jittery, &GAUSSIAN, 2000, spacing); + + // Keeping the order is not pacing the path: a datagram that drew more + // than the one in front falls further behind, and one that drew less + // closes up against it. + let gaps: Vec = sent.windows(2).map(|pair| pair[1].1 - pair[0].1).collect(); + assert!(gaps.iter().any(|&gap| gap < spacing), "nothing closed up"); + assert!(gaps.iter().any(|&gap| gap > spacing), "nothing fell behind"); + } + + #[test] + fn a_reorder_still_overtakes_gaussian_jitter() { + let shuffled = Profile { + delay: Duration::from_millis(5), + jitter: Duration::from_millis(50), + reorder: 0.05, + ..Default::default() + }; + let (sent, counters) = departures(&shuffled, &GAUSSIAN, 2000, Duration::from_millis(5)); + let overtaken = sent.windows(2).filter(|pair| pair[0].0 > pair[1].0).count(); + assert!(overtaken > 0, "a reorder never overtook"); + assert!(counters.reordered > 0, "{counters}"); + } + + #[test] + fn the_seed_reproduces_the_gaussian_decisions() { + let everything = Profile { + delay: Duration::from_millis(5), + jitter: Duration::from_millis(5), + loss: 0.05, + reorder: 0.05, + ..Default::default() + }; + let (first, first_counters) = departures(&everything, &GAUSSIAN, 1000, Duration::from_millis(1)); + let (second, second_counters) = departures(&everything, &GAUSSIAN, 1000, Duration::from_millis(1)); + + // The same ids survive and leave at the same offsets from their start. + let offsets = |sent: &[(u32, Instant)]| { + let start = sent.iter().map(|&(_, at)| at).min().unwrap(); + sent.iter().map(|&(id, at)| (id, at - start)).collect::>() + }; + assert_eq!(offsets(&first), offsets(&second)); + assert_eq!(first_counters, second_counters); + assert!(first_counters.lost > 0 && first_counters.reordered > 0); + } + + #[tokio::test] + async fn a_gaussian_sigma_may_exceed_the_delay() { + // The uniform model refuses this; a gaussian clamps its draw at zero. + let wide = Profile { + delay: Duration::from_millis(5), + jitter: Duration::from_millis(10), + ..Default::default() + }; + let (shaper, client) = shaped(|config| Setup { + up: GAUSSIAN, + ..Config { up: wide, ..config }.into() + }) + .await; + assert_eq!(round_trip(&client, 20).await, (0..20).collect::>()); + shaper.verify().unwrap(); + } } diff --git a/rs/moq-shaper/src/main.rs b/rs/moq-shaper/src/main.rs index cabb72f9ff..eeef1c158f 100644 --- a/rs/moq-shaper/src/main.rs +++ b/rs/moq-shaper/src/main.rs @@ -22,9 +22,12 @@ struct Args { /// The base one-way delay, e.g. `20ms`. #[arg(long, default_value = "0s", value_parser = humantime::parse_duration)] delay: Duration, - /// The most the delay varies, uniformly either way. + /// How far the delay varies: the most either way, or the sigma of a gaussian. #[arg(long, default_value = "0s", value_parser = humantime::parse_duration)] jitter: Duration, + /// How the jitter is drawn. + #[arg(long, value_enum, default_value_t = JitterModel::Uniform)] + jitter_model: JitterModel, /// The probability that a datagram is dropped. #[arg(long, default_value_t = 0.0)] loss: f64, @@ -45,6 +48,14 @@ struct Args { tcp_passthrough: bool, } +#[derive(Clone, Copy, clap::ValueEnum)] +enum JitterModel { + /// Uniform either way of the delay, per datagram, so datagrams can overtake each other. + Uniform, + /// A gaussian clamped at zero that never lets a datagram overtake the one in front. + Gaussian, +} + #[tokio::main] async fn main() -> anyhow::Result<()> { let args = Args::parse(); @@ -67,8 +78,16 @@ async fn main() -> anyhow::Result<()> { up: profile.clone(), down: profile, }; + let options = moq_shaper::Options { + jitter_model: match args.jitter_model { + JitterModel::Uniform => moq_shaper::Jitter::Uniform, + JitterModel::Gaussian => moq_shaper::Jitter::Gaussian, + }, + }; let shaper = moq_shaper::Shaper::bind(moq_shaper::Setup { tcp_passthrough: args.tcp_passthrough, + up: options.clone(), + down: options.clone(), ..config.into() }) .await?; @@ -81,6 +100,9 @@ async fn main() -> anyhow::Result<()> { config.seed, config.up ); + if options != moq_shaper::Options::default() { + println!("shaper: options {options:?}"); + } shutdown().await?; From 6b7964c1558803b9a8d948b853c0b1dca123ef03 Mon Sep 17 00:00:00 2001 From: fperex <270788582+fperex@users.noreply.github.com> Date: Fri, 25 Sep 2026 07:05:27 -0400 Subject: [PATCH 04/19] feat(shaper): one link each way for every client, as an option Each client flow takes a link of its own each way, with its own seeded stream, rate limit and queue. That models a path per client. A browser page is several clients on one path: its own WebTransport session and its audio worker's, on one access link. There, the second flow's datagrams wait behind the first's, and a clump holds both. `Setup::shared` gives every flow one link each way, which is how the path the audio-quality lane was measured on behaved. A jitter that keeps the order then keeps it across clients, and one seeded stream and one rate limit serve them all. The shared links draw the streams the first flow's would, and a client's treatment now depends on how its datagrams interleave with the others'. The CLI takes it as `--shared`. Off by default, so each flow still gets its own. To route a shared link's output, a treated datagram carries the socket and address it leaves by, and `Link::treat` returns a departure without queueing it, so the link's decisions can be tested without sockets. Co-Authored-By: Claude Opus 5.5 --- rs/moq-shaper/src/lib.rs | 227 +++++++++++++++++++++++++++++--------- rs/moq-shaper/src/main.rs | 8 +- 2 files changed, 182 insertions(+), 53 deletions(-) diff --git a/rs/moq-shaper/src/lib.rs b/rs/moq-shaper/src/lib.rs index c9d54744ec..9a915854f5 100644 --- a/rs/moq-shaper/src/lib.rs +++ b/rs/moq-shaper/src/lib.rs @@ -20,7 +20,7 @@ use std::{ fmt, net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}, sync::{ - Arc, OnceLock, + Arc, Mutex, OnceLock, atomic::{AtomicU64, Ordering}, }, time::Duration, @@ -198,6 +198,13 @@ pub struct Setup { /// never impaired: a reliable transport cannot shed load, so shaping it /// would measure how TCP retransmits. pub tcp_passthrough: bool, + /// Every client shares one link each way, the way clients behind one access + /// link do, rather than each getting its own. + /// + /// Shared, a jitter that keeps the order keeps it across clients, and one + /// seeded stream and one rate limit serve them all, so a client's treatment + /// depends on how its datagrams interleave with the others'. + pub shared: bool, /// The options from a client toward the target. pub up: Options, /// The options from the target back toward a client. @@ -209,6 +216,7 @@ impl From for Setup { Self { config, tcp_passthrough: false, + shared: false, up: Options::default(), down: Options::default(), } @@ -390,7 +398,8 @@ fn bump(counter: &AtomicU64) { /// Accept datagrams from clients, giving each client its own flow. /// /// A flow is a socket of its own toward the target, so the target sees one -/// address per client just as it would without the shaper in the way. +/// address per client just as it would without the shaper in the way. Each +/// flow takes a link of its own each way, unless the path is shared. async fn run( listen: Arc, tcp: Option, @@ -398,10 +407,19 @@ async fn run( tally: Arc<[Tally; 2]>, ) -> anyhow::Result<()> { let config = &setup.config; - let mut flows = HashMap::::new(); + let mut flows = HashMap::::new(); let mut tasks = JoinSet::new(); let mut buf = vec![0u8; u16::MAX as usize]; + // A shared path is one link each way, drawing the streams the first flow would. + let shared = setup.shared.then(|| { + let now = Instant::now(); + [ + link(&mut tasks, &config.up, &setup.up, config.seed, 0, now), + link(&mut tasks, &config.down, &setup.down, config.seed, 1, now), + ] + }); + loop { let (size, from) = tokio::select! { res = listen.recv_from(&mut buf) => res.context("receive from a client")?, @@ -418,27 +436,56 @@ async fn run( let now = Instant::now(); let stream = 2 * flows.len() as u64; - let link = match flows.entry(from) { + let flow = match flows.entry(from) { hash_map::Entry::Occupied(entry) => entry.into_mut(), hash_map::Entry::Vacant(entry) => { let upstream = Arc::new(bind_toward(config.target).await?); - let (up_tx, up_rx) = mpsc::unbounded_channel(); - tasks.spawn(deliver(up_rx, upstream.clone(), config.target)); - - let (down_tx, down_rx) = mpsc::unbounded_channel(); - tasks.spawn(deliver(down_rx, listen.clone(), from)); - - let down = Link::new(&config.down, &setup.down, config.seed, stream + 1, now, down_tx); - tasks.spawn(reply(upstream, config.target, down, tally.clone())); - - entry.insert(Link::new(&config.up, &setup.up, config.seed, stream, now, up_tx)) + let [up, down] = match &shared { + Some(links) => links.clone(), + None => [ + link(&mut tasks, &config.up, &setup.up, config.seed, stream, now), + link(&mut tasks, &config.down, &setup.down, config.seed, stream + 1, now), + ], + }; + tasks.spawn(reply( + upstream.clone(), + config.target, + down, + listen.clone(), + from, + tally.clone(), + )); + + entry.insert(Flow { upstream, up }) } }; - link.push(now, buf[..size].to_vec(), &tally[UP]); + let datagram = buf[..size].to_vec(); + let mut up = flow.up.lock().expect("link poisoned"); + up.push(now, datagram, &flow.upstream, config.target, &tally[UP]); } } +/// One client: its socket toward the target, and the link it sends through. +struct Flow { + upstream: Arc, + up: Arc>, +} + +/// A link, and the task delivering what it treats. +fn link( + tasks: &mut JoinSet>, + profile: &Profile, + options: &Options, + seed: u64, + stream: u64, + now: Instant, +) -> Arc> { + let (queue, queued) = mpsc::unbounded_channel(); + tasks.spawn(deliver(queued)); + Arc::new(Mutex::new(Link::new(profile, options, seed, stream, now, queue))) +} + /// The next TCP connection, or never without a passthrough. async fn accept(tcp: Option<&TcpListener>) -> std::io::Result<(TcpStream, SocketAddr)> { match tcp { @@ -470,11 +517,13 @@ async fn bind_toward(target: SocketAddr) -> anyhow::Result { .with_context(|| format!("bind a socket toward {target}")) } -/// Feed what the target sends a flow into that flow's return link. +/// Feed what the target sends a flow into the link back toward its client. async fn reply( socket: Arc, target: SocketAddr, - mut link: Link, + link: Arc>, + listen: Arc, + client: SocketAddr, tally: Arc<[Tally; 2]>, ) -> anyhow::Result<()> { let mut buf = vec![0u8; u16::MAX as usize]; @@ -484,19 +533,17 @@ async fn reply( if from != target { continue; } - link.push(Instant::now(), buf[..size].to_vec(), &tally[DOWN]); + let datagram = buf[..size].to_vec(); + let mut down = link.lock().expect("link poisoned"); + down.push(Instant::now(), datagram, &listen, client, &tally[DOWN]); } } /// Send each treated datagram at its departure time. -async fn deliver( - mut queue: mpsc::UnboundedReceiver<(Instant, Vec)>, - socket: Arc, - dest: SocketAddr, -) -> anyhow::Result<()> { +async fn deliver(mut queue: mpsc::UnboundedReceiver) -> anyhow::Result<()> { // Ordered by departure, then by arrival, so ties keep their order. let mut pending = BinaryHeap::>::new(); - let mut datagrams = HashMap::>::new(); + let mut parcels = HashMap::::new(); let mut sequence = 0u64; loop { @@ -504,9 +551,9 @@ async fn deliver( tokio::select! { item = queue.recv() => { // The link is gone, so the flow is too. - let Some((at, datagram)) = item else { return Ok(()) }; - pending.push(Reverse((at, sequence))); - datagrams.insert(sequence, datagram); + let Some(parcel) = item else { return Ok(()) }; + pending.push(Reverse((parcel.at, sequence))); + parcels.insert(sequence, parcel); sequence += 1; } _ = tokio::time::sleep_until(next.unwrap_or_else(Instant::now)), if next.is_some() => { @@ -516,18 +563,27 @@ async fn deliver( break; } pending.pop(); - let datagram = datagrams.remove(&id).expect("queued datagram"); + let parcel = parcels.remove(&id).expect("queued datagram"); // A send error is the path losing the datagram, the way a // network does when the far end is gone (a killed relay, say), // so it is not the shaper failing. - let _ = socket.send_to(&datagram, dest).await; + let _ = parcel.socket.send_to(&parcel.datagram, parcel.dest).await; } } } } } -/// One direction of one flow: the decisions and the state they depend on. +/// A treated datagram: when it leaves, and the socket and address it leaves by. +struct Parcel { + at: Instant, + datagram: Vec, + socket: Arc, + dest: SocketAddr, +} + +/// One direction of one flow, or of every flow on a shared path: the decisions +/// and the state they depend on. struct Link { profile: Profile, jitter: Jitter, @@ -536,21 +592,21 @@ struct Link { full_at: Instant, /// When the latest in-order datagram leaves, which the next one never beats. floor: Instant, - queue: mpsc::UnboundedSender<(Instant, Vec)>, + queue: mpsc::UnboundedSender, } impl Link { - /// Each direction of each flow draws from its own stream of the one seed, - /// so a flow's decisions do not depend on how its datagrams interleave with - /// anyone else's. Flows are numbered in the order they first send, since a - /// client's ephemeral port is no identity across runs. + /// Each link draws from its own stream of the one seed, so a flow's + /// decisions do not depend on how its datagrams interleave with anyone + /// else's, unless the path is shared. Flows are numbered in the order they + /// first send, since a client's ephemeral port is no identity across runs. fn new( profile: &Profile, options: &Options, seed: u64, stream: u64, now: Instant, - queue: mpsc::UnboundedSender<(Instant, Vec)>, + queue: mpsc::UnboundedSender, ) -> Self { Self { profile: profile.clone(), @@ -564,7 +620,23 @@ impl Link { } } - fn push(&mut self, now: Instant, datagram: Vec, tally: &Tally) { + /// Treat a datagram and queue it to leave by `socket` for `dest`. + fn push(&mut self, now: Instant, datagram: Vec, socket: &Arc, dest: SocketAddr, tally: &Tally) { + let Some(at) = self.treat(now, datagram.len(), tally) else { + return; + }; + // The delivery task only ends once this link is dropped. + let _ = self.queue.send(Parcel { + at, + datagram, + socket: socket.clone(), + dest, + }); + } + + /// When a datagram of `size` bytes arriving `now` leaves, or `None` when it + /// is dropped. + fn treat(&mut self, now: Instant, size: usize, tally: &Tally) -> Option { bump(&tally.packets); // Every draw happens for every datagram, whatever the profile, so one @@ -578,20 +650,20 @@ impl Link { if lose { bump(&tally.lost); - return; + return None; } let mut depart = now; if let Some(rate) = &self.profile.rate { // GCRA: a datagram may leave once the backlog, itself included, is // within the burst allowance. - let full_at = self.full_at.max(now) + rate.cost(datagram.len() as u64); + let full_at = self.full_at.max(now) + rate.cost(size as u64); let start = full_at .checked_sub(rate.cost(rate.burst)) .map_or(now, |start| start.max(now)); if start - now > rate.queue { bump(&tally.overflowed); - return; + return None; } if start > now { bump(&tally.throttled); @@ -619,8 +691,7 @@ impl Link { bump(&tally.delayed); } - // The delivery task only ends once this link is dropped. - let _ = self.queue.send((depart, datagram)); + Some(depart) } } @@ -947,18 +1018,13 @@ mod tests { count: u32, spacing: Duration, ) -> (Vec<(u32, Instant)>, Counters) { - let (queue, mut queued) = mpsc::unbounded_channel(); + let (queue, _) = mpsc::unbounded_channel(); let now = Instant::now(); let mut link = Link::new(profile, options, 7, 0, now, queue); let tally = Tally::default(); - for id in 0..count { - link.push(now + spacing * id, id.to_be_bytes().to_vec(), &tally); - } - - let mut sent = Vec::new(); - while let Ok((at, datagram)) = queued.try_recv() { - sent.push((u32::from_be_bytes(datagram[..4].try_into().unwrap()), at)); - } + let mut sent: Vec<(u32, Instant)> = (0..count) + .filter_map(|id| Some((id, link.treat(now + spacing * id, 4, &tally)?))) + .collect(); // Ties leave in arrival order, which is id order here. sent.sort_by_key(|&(id, at)| (at, id)); (sent, tally.snapshot()) @@ -1102,4 +1168,63 @@ mod tests { assert_eq!(round_trip(&client, 20).await, (0..20).collect::>()); shaper.verify().unwrap(); } + + /// Two clients take turns sending numbered datagrams up one gaussian path, + /// and the target's arrival order comes back. + async fn two_clients(shared: bool) -> Vec { + let target = UdpSocket::bind(LOCALHOST).await.unwrap(); + let config = Config { + bind: LOCALHOST, + target: target.local_addr().unwrap(), + seed: 5, + up: Profile { + delay: Duration::from_millis(20), + jitter: Duration::from_millis(10), + ..Default::default() + }, + down: Profile::default(), + }; + let shaper = Shaper::bind(Setup { + shared, + up: GAUSSIAN, + ..config.into() + }) + .await + .unwrap(); + + let clients = [ + UdpSocket::bind(LOCALHOST).await.unwrap(), + UdpSocket::bind(LOCALHOST).await.unwrap(), + ]; + for id in 0..200u32 { + let client = &clients[id as usize % 2]; + client.send_to(&id.to_be_bytes(), shaper.addr()).await.unwrap(); + } + + let mut got = Vec::new(); + let mut buf = [0u8; 4]; + while let Ok(Ok(_)) = tokio::time::timeout(Duration::from_millis(200), target.recv_from(&mut buf)).await { + got.push(u32::from_be_bytes(buf)); + } + got + } + + #[tokio::test] + async fn a_shared_path_keeps_the_order_across_clients() { + // Separate links keep each client's order, but not the order between them. + let apart = two_clients(false).await; + let mut sorted = apart.clone(); + sorted.sort(); + assert_eq!(sorted, (0..200).collect::>(), "a client lost datagrams"); + assert_ne!( + apart, sorted, + "separate links kept the order anyway, so this proves nothing" + ); + for parity in 0..2 { + assert!(apart.iter().filter(|&&id| id % 2 == parity).is_sorted()); + } + + let together = two_clients(true).await; + assert_eq!(together, (0..200).collect::>(), "a shared path reordered"); + } } diff --git a/rs/moq-shaper/src/main.rs b/rs/moq-shaper/src/main.rs index eeef1c158f..e8585c23b5 100644 --- a/rs/moq-shaper/src/main.rs +++ b/rs/moq-shaper/src/main.rs @@ -43,6 +43,9 @@ struct Args { /// The longest a datagram waits for the rate limit before it is dropped. #[arg(long, default_value = "100ms", value_parser = humantime::parse_duration, requires = "rate")] queue: Duration, + /// Every client shares one link each way, instead of each getting its own. + #[arg(long)] + shared: bool, /// Also pipe TCP on the listening port to the target, untouched. #[arg(long)] tcp_passthrough: bool, @@ -86,6 +89,7 @@ async fn main() -> anyhow::Result<()> { }; let shaper = moq_shaper::Shaper::bind(moq_shaper::Setup { tcp_passthrough: args.tcp_passthrough, + shared: args.shared, up: options.clone(), down: options.clone(), ..config.into() @@ -100,8 +104,8 @@ async fn main() -> anyhow::Result<()> { config.seed, config.up ); - if options != moq_shaper::Options::default() { - println!("shaper: options {options:?}"); + if options != moq_shaper::Options::default() || args.shared { + println!("shaper: options {options:?}, shared {}", args.shared); } shutdown().await?; From 7e7b7290500b007222db852f4dd8db2923e5deab Mon Sep 17 00:00:00 2001 From: fperex <270788582+fperex@users.noreply.github.com> Date: Fri, 25 Sep 2026 07:10:34 -0400 Subject: [PATCH 05/19] feat(shaper): batches that release datagrams together, as an option A paced hop, a Wi-Fi access point or a cellular scheduler bunches traffic: nothing moves for a while, then several datagrams arrive at once. That clump is what a receiver's jitter estimate has to see, and no delay or jitter produces it, since those make datagrams late rather than together. `Options::batch` holds each direction's datagrams until `count` are waiting, or until `window` after the first of them arrived, then sends the whole batch when its latest datagram would have left, and never before the batch closed. The delivery task holds it, so on a shared path one batch fills from every client. A hold counts as `delayed`, once, and `verify` fails a run whose batches implausibly never held anything, which a batch of `count` does to all but one datagram in `count`. The CLI takes `--batch --batch-window `. Off by default. Co-Authored-By: Claude Opus 5.5 --- rs/moq-shaper/src/lib.rs | 447 +++++++++++++++++++++++++++++++++----- rs/moq-shaper/src/main.rs | 10 + 2 files changed, 401 insertions(+), 56 deletions(-) diff --git a/rs/moq-shaper/src/lib.rs b/rs/moq-shaper/src/lib.rs index 9a915854f5..2b31df9dae 100644 --- a/rs/moq-shaper/src/lib.rs +++ b/rs/moq-shaper/src/lib.rs @@ -102,17 +102,38 @@ pub enum Jitter { Gaussian, } +/// Hold datagrams, then release them together, the way a paced hop bunches them. +/// +/// A batch closes once `count` datagrams are waiting, or `window` after the +/// first of them arrived, and everything in it leaves when the latest would. +/// That clump is what a receiver's jitter estimate sees on such a path, and no +/// delay or jitter produces it: the datagrams arrive together rather than late. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct Batch { + /// How many datagrams close a batch early. + pub count: usize, + /// How long a batch waits for that many. + pub window: Duration, +} + /// One direction's opt-in options beyond its [`Profile`]. The default adds nothing. #[derive(Clone, Debug, Default, PartialEq)] pub struct Options { /// How the profile's `jitter` is drawn. pub jitter_model: Jitter, + /// Hold datagrams and release them together, if at all. + pub batch: Option, } impl Options { /// Check the options, and `profile` as they draw it. fn validate(&self, profile: &Profile) -> anyhow::Result<()> { - profile.validate(self.jitter_model) + profile.validate(self.jitter_model)?; + if let Some(batch) = &self.batch { + anyhow::ensure!(batch.count > 0, "a batch of zero datagrams never closes"); + anyhow::ensure!(!batch.window.is_zero(), "a batch with no window never holds anything"); + } + Ok(()) } } @@ -150,6 +171,18 @@ fn unapplied(config: &Config, stats: &Stats) -> Vec<&'static str> { .collect() } +/// Whether the batches `setup` configures implausibly never held a datagram. +/// +/// A batch holds every datagram but the one that fills it, so it acts on at +/// least `1 - 1 / count` of them. What it holds counts as delayed once the +/// batch leaves. +fn unbatched(setup: &Setup, stats: &Stats) -> bool { + let chance = |options: &Options| options.batch.map_or(0.0, |batch| 1.0 - 1.0 / batch.count as f64); + let silence = (1.0 - chance(&setup.up)).powf(stats.up.packets as f64) + * (1.0 - chance(&setup.down)).powf(stats.down.packets as f64); + stats.up.delayed + stats.down.delayed == 0 && silence < IMPLAUSIBLE +} + /// A token-bucket rate limit with a bounded queue behind it. #[derive(Clone, Debug, PartialEq)] pub struct Rate { @@ -234,7 +267,7 @@ pub struct Counters { pub overflowed: u64, /// Datagrams that waited for the rate limit. pub throttled: u64, - /// Datagrams given a nonzero delay. + /// Datagrams given a nonzero delay, by the profile or by a batch holding them. pub delayed: u64, /// Datagrams sent ahead of the delay, overtaking any still in flight. pub reordered: u64, @@ -349,7 +382,10 @@ impl Shaper { anyhow::bail!("the shaper stopped forwarding: {err} ({stats})"); } - let missing = unapplied(&self.setup.config, &stats); + let mut missing = unapplied(&self.setup.config, &stats); + if unbatched(&self.setup, &stats) { + missing.push("batch"); + } anyhow::ensure!( missing.is_empty(), "the profile never applied {} ({stats}), so this run was not impaired as configured", @@ -414,10 +450,7 @@ async fn run( // A shared path is one link each way, drawing the streams the first flow would. let shared = setup.shared.then(|| { let now = Instant::now(); - [ - link(&mut tasks, &config.up, &setup.up, config.seed, 0, now), - link(&mut tasks, &config.down, &setup.down, config.seed, 1, now), - ] + [UP, DOWN].map(|direction| link(&mut tasks, &setup, direction, 0, now, &tally)) }); loop { @@ -435,7 +468,7 @@ async fn run( }; let now = Instant::now(); - let stream = 2 * flows.len() as u64; + let number = flows.len() as u64; let flow = match flows.entry(from) { hash_map::Entry::Occupied(entry) => entry.into_mut(), hash_map::Entry::Vacant(entry) => { @@ -443,10 +476,7 @@ async fn run( let [up, down] = match &shared { Some(links) => links.clone(), - None => [ - link(&mut tasks, &config.up, &setup.up, config.seed, stream, now), - link(&mut tasks, &config.down, &setup.down, config.seed, stream + 1, now), - ], + None => [UP, DOWN].map(|direction| link(&mut tasks, &setup, direction, number, now, &tally)), }; tasks.spawn(reply( upstream.clone(), @@ -472,18 +502,31 @@ struct Flow { up: Arc>, } -/// A link, and the task delivering what it treats. +/// One direction of the flow numbered `flow` as a link, and the task delivering +/// what it treats. fn link( tasks: &mut JoinSet>, - profile: &Profile, - options: &Options, - seed: u64, - stream: u64, + setup: &Setup, + direction: usize, + flow: u64, now: Instant, + tally: &Arc<[Tally; 2]>, ) -> Arc> { + let (profile, options) = match direction { + UP => (&setup.config.up, &setup.up), + _ => (&setup.config.down, &setup.down), + }; let (queue, queued) = mpsc::unbounded_channel(); - tasks.spawn(deliver(queued)); - Arc::new(Mutex::new(Link::new(profile, options, seed, stream, now, queue))) + tasks.spawn(deliver(queued, options.batch, tally.clone(), direction)); + let stream = 2 * flow + direction as u64; + Arc::new(Mutex::new(Link::new( + profile, + options, + setup.config.seed, + stream, + now, + queue, + ))) } /// The next TCP connection, or never without a passthrough. @@ -539,24 +582,34 @@ async fn reply( } } -/// Send each treated datagram at its departure time. -async fn deliver(mut queue: mpsc::UnboundedReceiver) -> anyhow::Result<()> { +/// Send each treated datagram at its departure time, after its batch if the +/// link has one. +async fn deliver( + mut queue: mpsc::UnboundedReceiver, + batch: Option, + tally: Arc<[Tally; 2]>, + direction: usize, +) -> anyhow::Result<()> { // Ordered by departure, then by arrival, so ties keep their order. let mut pending = BinaryHeap::>::new(); let mut parcels = HashMap::::new(); let mut sequence = 0u64; + let mut held = Held::default(); + let mut ready = Vec::new(); loop { let next = pending.peek().map(|Reverse((at, _))| *at); + let wake = next.into_iter().chain(held.closes).min(); tokio::select! { item = queue.recv() => { // The link is gone, so the flow is too. let Some(parcel) = item else { return Ok(()) }; - pending.push(Reverse((parcel.at, sequence))); - parcels.insert(sequence, parcel); - sequence += 1; + match batch { + Some(batch) => held.hold(batch, parcel, &mut ready, &tally[direction]), + None => ready.push(parcel), + } } - _ = tokio::time::sleep_until(next.unwrap_or_else(Instant::now)), if next.is_some() => { + _ = tokio::time::sleep_until(wake.unwrap_or_else(Instant::now)), if wake.is_some() => { let now = Instant::now(); while let Some(&Reverse((at, id))) = pending.peek() { if at > now { @@ -569,14 +622,64 @@ async fn deliver(mut queue: mpsc::UnboundedReceiver) -> anyhow::Result<( // so it is not the shaper failing. let _ = parcel.socket.send_to(&parcel.datagram, parcel.dest).await; } + // The window closes a batch that never filled. + if let Some(closes) = held.closes.filter(|&closes| closes <= now) { + held.release(closes, &mut ready, &tally[direction]); + } } } + for parcel in ready.drain(..) { + pending.push(Reverse((parcel.at, sequence))); + parcels.insert(sequence, parcel); + sequence += 1; + } } } -/// A treated datagram: when it leaves, and the socket and address it leaves by. +/// The datagrams a batch is holding, and when its window closes on them. +#[derive(Default)] +struct Held { + parcels: Vec, + closes: Option, +} + +impl Held { + /// Hold `parcel`, releasing the batch into `ready` once it is full. + fn hold(&mut self, batch: Batch, parcel: Parcel, ready: &mut Vec, tally: &Tally) { + let arrived = parcel.arrived; + self.closes.get_or_insert(arrived + batch.window); + self.parcels.push(parcel); + if self.parcels.len() >= batch.count { + self.release(arrived, ready, tally); + } + } + + /// Release the batch, closed at `closed`, into `ready`: every datagram leaves + /// when the latest would, and none before the batch closed. + fn release(&mut self, closed: Instant, ready: &mut Vec, tally: &Tally) { + self.closes = None; + let Some(latest) = self.parcels.iter().map(|parcel| parcel.at).max() else { + return; + }; + let at = latest.max(closed); + for mut parcel in self.parcels.drain(..) { + // A hold is a delay, counted once whichever stage gave it. + if at > parcel.at && !parcel.delayed { + bump(&tally.delayed); + } + parcel.at = at; + ready.push(parcel); + } + } +} + +/// A treated datagram: when it arrived and leaves, and the socket and address +/// it leaves by. struct Parcel { + arrived: Instant, at: Instant, + /// Whether the link already counted it as delayed. + delayed: bool, datagram: Vec, socket: Arc, dest: SocketAddr, @@ -622,21 +725,23 @@ impl Link { /// Treat a datagram and queue it to leave by `socket` for `dest`. fn push(&mut self, now: Instant, datagram: Vec, socket: &Arc, dest: SocketAddr, tally: &Tally) { - let Some(at) = self.treat(now, datagram.len(), tally) else { + let Some((at, delayed)) = self.treat(now, datagram.len(), tally) else { return; }; // The delivery task only ends once this link is dropped. let _ = self.queue.send(Parcel { + arrived: now, at, + delayed, datagram, socket: socket.clone(), dest, }); } - /// When a datagram of `size` bytes arriving `now` leaves, or `None` when it - /// is dropped. - fn treat(&mut self, now: Instant, size: usize, tally: &Tally) -> Option { + /// When a datagram of `size` bytes arriving `now` leaves, and whether it was + /// counted as delayed, or `None` when it is dropped. + fn treat(&mut self, now: Instant, size: usize, tally: &Tally) -> Option<(Instant, bool)> { bump(&tally.packets); // Every draw happens for every datagram, whatever the profile, so one @@ -672,6 +777,7 @@ impl Link { depart = start; } + let mut delayed = false; if skip { bump(&tally.reordered); } else if self.jitter == Jitter::Gaussian { @@ -680,18 +786,19 @@ impl Link { let leave = (depart + Duration::from_secs_f64(wait.max(0.0))).max(self.floor); // Counted like the uniform model whenever a delay is configured, so // the chance `verify` holds it to is the same. - if !self.profile.delay.is_zero() || leave > depart { - bump(&tally.delayed); - } + delayed = !self.profile.delay.is_zero() || leave > depart; self.floor = leave; depart = leave; } else if !self.profile.delay.is_zero() { let jitter = self.profile.jitter.as_secs_f64() * spread; depart += Duration::from_secs_f64(self.profile.delay.as_secs_f64() + jitter); + delayed = true; + } + if delayed { bump(&tally.delayed); } - Some(depart) + Some((depart, delayed)) } } @@ -1023,7 +1130,7 @@ mod tests { let mut link = Link::new(profile, options, 7, 0, now, queue); let tally = Tally::default(); let mut sent: Vec<(u32, Instant)> = (0..count) - .filter_map(|id| Some((id, link.treat(now + spacing * id, 4, &tally)?))) + .filter_map(|id| Some((id, link.treat(now + spacing * id, 4, &tally)?.0))) .collect(); // Ties leave in arrival order, which is id order here. sent.sort_by_key(|&(id, at)| (at, id)); @@ -1032,6 +1139,7 @@ mod tests { const GAUSSIAN: Options = Options { jitter_model: Jitter::Gaussian, + batch: None, }; #[tokio::test] @@ -1169,50 +1277,63 @@ mod tests { shaper.verify().unwrap(); } - /// Two clients take turns sending numbered datagrams up one gaussian path, - /// and the target's arrival order comes back. - async fn two_clients(shared: bool) -> Vec { + /// Two clients take turns sending `count` numbered datagrams up the path + /// `setup` builds, and what reaches the target comes back, in order, with + /// how long after the first send it did. + async fn two_clients(setup: impl FnOnce(Config) -> Setup, count: u32) -> Vec<(u32, Duration)> { let target = UdpSocket::bind(LOCALHOST).await.unwrap(); let config = Config { bind: LOCALHOST, target: target.local_addr().unwrap(), seed: 5, - up: Profile { - delay: Duration::from_millis(20), - jitter: Duration::from_millis(10), - ..Default::default() - }, + up: Profile::default(), down: Profile::default(), }; - let shaper = Shaper::bind(Setup { - shared, - up: GAUSSIAN, - ..config.into() - }) - .await - .unwrap(); + let shaper = Shaper::bind(setup(config)).await.unwrap(); let clients = [ UdpSocket::bind(LOCALHOST).await.unwrap(), UdpSocket::bind(LOCALHOST).await.unwrap(), ]; - for id in 0..200u32 { + let start = Instant::now(); + for id in 0..count { let client = &clients[id as usize % 2]; client.send_to(&id.to_be_bytes(), shaper.addr()).await.unwrap(); } let mut got = Vec::new(); let mut buf = [0u8; 4]; - while let Ok(Ok(_)) = tokio::time::timeout(Duration::from_millis(200), target.recv_from(&mut buf)).await { - got.push(u32::from_be_bytes(buf)); + while let Ok(Ok(_)) = tokio::time::timeout(Duration::from_millis(500), target.recv_from(&mut buf)).await { + got.push((u32::from_be_bytes(buf), start.elapsed())); } got } #[tokio::test] async fn a_shared_path_keeps_the_order_across_clients() { + let jittery = Profile { + delay: Duration::from_millis(20), + jitter: Duration::from_millis(10), + ..Default::default() + }; + let order = |shared: bool| { + let up = jittery.clone(); + async move { + let got = two_clients( + |config| Setup { + shared, + up: GAUSSIAN, + ..Config { up, ..config }.into() + }, + 200, + ) + .await; + got.into_iter().map(|(id, _)| id).collect::>() + } + }; + // Separate links keep each client's order, but not the order between them. - let apart = two_clients(false).await; + let apart = order(false).await; let mut sorted = apart.clone(); sorted.sort(); assert_eq!(sorted, (0..200).collect::>(), "a client lost datagrams"); @@ -1224,7 +1345,221 @@ mod tests { assert!(apart.iter().filter(|&&id| id % 2 == parity).is_sorted()); } - let together = two_clients(true).await; - assert_eq!(together, (0..200).collect::>(), "a shared path reordered"); + assert_eq!( + order(true).await, + (0..200).collect::>(), + "a shared path reordered" + ); + } + + /// Send `count` numbered datagrams `spacing` apart, and collect what echoes + /// back with when it did, alongside when each was sent. + async fn paced(client: &UdpSocket, count: u32, spacing: Duration) -> (Vec, Vec<(u32, Instant)>) { + let send = async { + let mut sent = Vec::new(); + for id in 0..count { + sent.push(Instant::now()); + client.send(&id.to_be_bytes()).await.unwrap(); + tokio::time::sleep(spacing).await; + } + sent + }; + let receive = async { + let mut got = Vec::new(); + let mut buf = [0u8; 4]; + while let Ok(Ok(_)) = tokio::time::timeout(Duration::from_millis(500), client.recv(&mut buf)).await { + got.push((u32::from_be_bytes(buf), Instant::now())); + } + got + }; + tokio::join!(send, receive) + } + + fn batched(count: usize, window: Duration) -> Options { + Options { + batch: Some(Batch { count, window }), + ..Default::default() + } + } + + #[tokio::test] + async fn a_batch_releases_on_count_and_on_the_window() { + let socket = Arc::new(UdpSocket::bind(LOCALHOST).await.unwrap()); + let ms = |ms| Duration::from_millis(ms); + let now = Instant::now(); + let parcel = |at: Instant| Parcel { + arrived: at, + at, + delayed: false, + datagram: Vec::new(), + socket: socket.clone(), + dest: LOCALHOST, + }; + let batch = Batch { + count: 3, + window: ms(160), + }; + let tally = Tally::default(); + let mut held = Held::default(); + let mut ready = Vec::new(); + + held.hold(batch, parcel(now), &mut ready, &tally); + held.hold(batch, parcel(now + ms(10)), &mut ready, &tally); + assert!(ready.is_empty(), "a partial batch leaked"); + assert_eq!( + held.closes, + Some(now + ms(160)), + "the window counts from the first arrival" + ); + + held.hold(batch, parcel(now + ms(20)), &mut ready, &tally); + let leaves: Vec = ready.drain(..).map(|parcel| parcel.at).collect(); + assert_eq!( + leaves, + [now + ms(20); 3], + "a full batch leaves together, as the latest would" + ); + assert_eq!( + tally.snapshot().delayed, + 2, + "the datagram that filled it waited for nothing" + ); + + // The window closes a batch that never fills, and nothing in it leaves sooner. + held.hold(batch, parcel(now + ms(30)), &mut ready, &tally); + let closes = held.closes.unwrap(); + assert_eq!(closes, now + ms(190)); + held.release(closes, &mut ready, &tally); + assert_eq!(ready.iter().map(|parcel| parcel.at).collect::>(), [closes]); + assert_eq!(tally.snapshot().delayed, 3); + } + + #[tokio::test] + async fn a_batch_releases_datagrams_together() { + let (shaper, client) = shaped(|config| Setup { + up: batched(7, Duration::from_millis(160)), + ..config.into() + }) + .await; + + // Three full batches, each filled well inside its window. + let (_, got) = paced(&client, 21, Duration::from_millis(10)).await; + assert_eq!( + got.iter().map(|&(id, _)| id).collect::>(), + (0..21).collect::>() + ); + + let batches: Vec<&[(u32, Instant)]> = got.chunks(7).collect(); + for batch in &batches { + let spread = batch[6].1 - batch[0].1; + assert!( + spread < Duration::from_millis(15), + "a batch arrived spread over {spread:?}" + ); + } + for pair in batches.windows(2) { + let gap = pair[1][0].1 - pair[0][0].1; + assert!(gap > Duration::from_millis(40), "two batches arrived {gap:?} apart"); + } + + let stats = shaper.verify().unwrap(); + assert_eq!( + stats.up.delayed, 18, + "every datagram but the last of each batch waits: {stats}" + ); + } + + #[tokio::test] + async fn the_window_releases_a_batch_that_never_fills() { + let (shaper, client) = shaped(|config| Setup { + up: batched(100, Duration::from_millis(80)), + ..config.into() + }) + .await; + + let (sent, got) = paced(&client, 3, Duration::ZERO).await; + assert_eq!(got.len(), 3); + let held = got[0].1 - sent[0]; + assert!(held >= Duration::from_millis(80), "the batch left after only {held:?}"); + assert_eq!(shaper.verify().unwrap().up.delayed, 3); + } + + #[tokio::test] + async fn a_shared_batch_fills_from_every_client() { + // Three datagrams from each of two clients, into batches of six: only a + // shared path fills one, and separate links wait out the window. + let window = Duration::from_millis(300); + for shared in [true, false] { + let got = two_clients( + |config| Setup { + shared, + up: batched(6, window), + ..config.into() + }, + 6, + ) + .await; + assert_eq!(got.len(), 6); + let last = got.iter().map(|&(_, after)| after).max().unwrap(); + assert_eq!( + last < window / 2, + shared, + "shared {shared}: the batch left after {last:?}" + ); + } + } + + #[tokio::test] + async fn a_batch_needs_a_count_and_a_window() { + let refused = |options: Options, why: &'static str| async move { + let config = Config { + bind: LOCALHOST, + target: LOCALHOST, + seed: 0, + up: Profile::default(), + down: Profile::default(), + }; + let err = Shaper::bind(Setup { + up: options, + ..config.into() + }) + .await + .err() + .unwrap_or_else(|| panic!("accepted a batch where {why}")); + assert!(format!("{err:#}").contains(why), "{err:#}"); + }; + refused(batched(0, Duration::from_millis(10)), "never closes").await; + refused(batched(7, Duration::ZERO), "never holds").await; + } + + #[test] + fn a_batch_that_never_held_anything_is_unapplied() { + let config = Config { + bind: LOCALHOST, + target: LOCALHOST, + seed: 0, + up: Profile::default(), + down: Profile::default(), + }; + let setup = Setup { + up: batched(7, Duration::from_millis(160)), + ..config.into() + }; + let quiet = |packets| Stats { + up: Counters { + packets, + ..Default::default() + }, + ..Default::default() + }; + + // (1/7)^2 is 2%: two datagrams through batches of seven prove nothing. + assert!(!unbatched(&setup, &quiet(2))); + // (1/7)^10 is 4e-9: ten datagrams a batch never held means no batch. + assert!(unbatched(&setup, &quiet(10))); + + let mut held = quiet(10); + held.up.delayed = 6; + assert!(!unbatched(&setup, &held)); } } diff --git a/rs/moq-shaper/src/main.rs b/rs/moq-shaper/src/main.rs index e8585c23b5..6de67ba9e3 100644 --- a/rs/moq-shaper/src/main.rs +++ b/rs/moq-shaper/src/main.rs @@ -43,6 +43,12 @@ struct Args { /// The longest a datagram waits for the rate limit before it is dropped. #[arg(long, default_value = "100ms", value_parser = humantime::parse_duration, requires = "rate")] queue: Duration, + /// Hold datagrams until this many are waiting, then release them together. + #[arg(long, requires = "batch_window")] + batch: Option, + /// The longest a batch waits to fill before it leaves anyway. + #[arg(long, value_parser = humantime::parse_duration, requires = "batch")] + batch_window: Option, /// Every client shares one link each way, instead of each getting its own. #[arg(long)] shared: bool, @@ -86,6 +92,10 @@ async fn main() -> anyhow::Result<()> { JitterModel::Uniform => moq_shaper::Jitter::Uniform, JitterModel::Gaussian => moq_shaper::Jitter::Gaussian, }, + batch: args + .batch + .zip(args.batch_window) + .map(|(count, window)| moq_shaper::Batch { count, window }), }; let shaper = moq_shaper::Shaper::bind(moq_shaper::Setup { tcp_passthrough: args.tcp_passthrough, From b6230c5519b2b6b8778498de0ad6ded892945dd5 Mon Sep 17 00:00:00 2001 From: fperex <270788582+fperex@users.noreply.github.com> Date: Fri, 25 Sep 2026 07:13:41 -0400 Subject: [PATCH 06/19] feat(shaper): steps that change a profile part-way through a run A path that stays one shape for a whole run cannot show how a receiver adapts when it changes: a delay that grows, a rate that narrows, loss that comes and goes. A steady profile measures convergence only. `Options::steps` is a list of `Step`s, each at a time into the run, naming the delay, jitter, loss, reorder or rate limit it changes. A step changes only what it names, so a later one puts a knob back without restating the rest. Steps count from when forwarding started, so every flow steps at the same moment. A rate step keeps the old bucket's credit, or its debt, clipped to the new one, and a limit that was not there starts full. `bind` refuses steps out of order, a step that changes nothing, and any step that leaves the profile invalid. Steps have no CLI flags; a profile file names them. Off by default. Links now start their bucket and their order floor at the run's start rather than their first datagram. The bucket reads `full_at.max(now)` and the floor only ever rises, so an earlier start changes nothing. Co-Authored-By: Claude Opus 5.5 --- rs/moq-shaper/src/lib.rs | 351 ++++++++++++++++++++++++++++++++++++-- rs/moq-shaper/src/main.rs | 1 + 2 files changed, 339 insertions(+), 13 deletions(-) diff --git a/rs/moq-shaper/src/lib.rs b/rs/moq-shaper/src/lib.rs index 2b31df9dae..90b01f777e 100644 --- a/rs/moq-shaper/src/lib.rs +++ b/rs/moq-shaper/src/lib.rs @@ -116,6 +116,48 @@ pub struct Batch { pub window: Duration, } +/// A change to one direction's profile once the run reaches `at`. +/// +/// A step changes only what it names, so a later step puts one knob back +/// without restating the rest. It can add or change a rate limit, never remove +/// one. +#[derive(Clone, Debug, Default, PartialEq)] +pub struct Step { + /// How far into the run the change happens. + pub at: Duration, + /// The delay from then on. + pub delay: Option, + /// The jitter from then on. + pub jitter: Option, + /// The loss from then on. + pub loss: Option, + /// The reorder from then on. + pub reorder: Option, + /// The rate limit from then on. + pub rate: Option, +} + +impl Step { + /// Change what this step names in `profile`. + fn apply(&self, profile: &mut Profile) { + if let Some(delay) = self.delay { + profile.delay = delay; + } + if let Some(jitter) = self.jitter { + profile.jitter = jitter; + } + if let Some(loss) = self.loss { + profile.loss = loss; + } + if let Some(reorder) = self.reorder { + profile.reorder = reorder; + } + if let Some(rate) = &self.rate { + profile.rate = Some(rate.clone()); + } + } +} + /// One direction's opt-in options beyond its [`Profile`]. The default adds nothing. #[derive(Clone, Debug, Default, PartialEq)] pub struct Options { @@ -123,16 +165,44 @@ pub struct Options { pub jitter_model: Jitter, /// Hold datagrams and release them together, if at all. pub batch: Option, + /// Changes to the profile part-way through the run, earliest first. + pub steps: Vec, } impl Options { - /// Check the options, and `profile` as they draw it. + /// Check the options, and `profile` as they draw it, after every step too. fn validate(&self, profile: &Profile) -> anyhow::Result<()> { profile.validate(self.jitter_model)?; if let Some(batch) = &self.batch { anyhow::ensure!(batch.count > 0, "a batch of zero datagrams never closes"); anyhow::ensure!(!batch.window.is_zero(), "a batch with no window never holds anything"); } + + let mut profile = profile.clone(); + let mut previous: Option = None; + for step in &self.steps { + // A link only ever looks at the next step due, so one out of order + // would be skipped without a word. + anyhow::ensure!( + previous.is_none_or(|previous| step.at > previous), + "the step at {:?} comes after the one at {previous:?}; steps go in order", + step.at + ); + // A step naming nothing is a profile that thinks it changes and does not. + anyhow::ensure!( + Step { + at: step.at, + ..Step::default() + } != *step, + "the step at {:?} changes nothing", + step.at + ); + step.apply(&mut profile); + profile + .validate(self.jitter_model) + .with_context(|| format!("after the step at {:?}", step.at))?; + previous = Some(step.at); + } Ok(()) } } @@ -447,11 +517,13 @@ async fn run( let mut tasks = JoinSet::new(); let mut buf = vec![0u8; u16::MAX as usize]; + // What a step's `at` counts from. + let start = Instant::now(); + // A shared path is one link each way, drawing the streams the first flow would. - let shared = setup.shared.then(|| { - let now = Instant::now(); - [UP, DOWN].map(|direction| link(&mut tasks, &setup, direction, 0, now, &tally)) - }); + let shared = setup + .shared + .then(|| [UP, DOWN].map(|direction| link(&mut tasks, &setup, direction, 0, start, &tally))); loop { let (size, from) = tokio::select! { @@ -476,7 +548,7 @@ async fn run( let [up, down] = match &shared { Some(links) => links.clone(), - None => [UP, DOWN].map(|direction| link(&mut tasks, &setup, direction, number, now, &tally)), + None => [UP, DOWN].map(|direction| link(&mut tasks, &setup, direction, number, start, &tally)), }; tasks.spawn(reply( upstream.clone(), @@ -509,7 +581,7 @@ fn link( setup: &Setup, direction: usize, flow: u64, - now: Instant, + start: Instant, tally: &Arc<[Tally; 2]>, ) -> Arc> { let (profile, options) = match direction { @@ -524,7 +596,7 @@ fn link( options, setup.config.seed, stream, - now, + start, queue, ))) } @@ -695,6 +767,10 @@ struct Link { full_at: Instant, /// When the latest in-order datagram leaves, which the next one never beats. floor: Instant, + /// When the run started, which a step's `at` counts from. + start: Instant, + /// The steps still to come, soonest last. + steps: Vec, queue: mpsc::UnboundedSender, } @@ -708,21 +784,48 @@ impl Link { options: &Options, seed: u64, stream: u64, - now: Instant, + start: Instant, queue: mpsc::UnboundedSender, ) -> Self { Self { profile: profile.clone(), jitter: options.jitter_model, rng: Xoshiro256PlusPlus::seed_from_u64(seed ^ stream.wrapping_mul(0x9E37_79B9_7F4A_7C15)), - // Not the moment the link is built, which is after `now`: that would - // make the first datagram queue behind a bucket still refilling. - full_at: now, - floor: now, + // Full since the run started, never since the link was built, which + // is after its first datagram arrived: that would make the datagram + // queue behind a bucket still refilling. + full_at: start, + floor: start, + start, + steps: options.steps.iter().rev().cloned().collect(), queue, } } + /// Apply every step the run has reached by `now`, in order. + fn step(&mut self, now: Instant) { + while self.steps.last().is_some_and(|step| self.start + step.at <= now) { + let step = self.steps.pop().expect("a step is due"); + if let Some(rate) = &step.rate { + self.full_at = self.refilled(rate, now); + } + step.apply(&mut self.profile); + } + } + + /// When `rate`'s bucket would be full again if it took over at `now`. + /// + /// A limit that was not there starts full, the way the run started. One + /// that was keeps its credit, or its debt, clipped to the new bucket. + fn refilled(&self, rate: &Rate, now: Instant) -> Instant { + let Some(old) = &self.profile.rate else { + return now; + }; + let owed = self.full_at.saturating_duration_since(now).as_secs_f64() * old.bits_per_second as f64 / 8.0; + let credit = (old.burst as f64 - owed).min(rate.burst as f64); + now + Duration::from_secs_f64((rate.burst as f64 - credit) * 8.0 / rate.bits_per_second as f64) + } + /// Treat a datagram and queue it to leave by `socket` for `dest`. fn push(&mut self, now: Instant, datagram: Vec, socket: &Arc, dest: SocketAddr, tally: &Tally) { let Some((at, delayed)) = self.treat(now, datagram.len(), tally) else { @@ -742,6 +845,7 @@ impl Link { /// When a datagram of `size` bytes arriving `now` leaves, and whether it was /// counted as delayed, or `None` when it is dropped. fn treat(&mut self, now: Instant, size: usize, tally: &Tally) -> Option<(Instant, bool)> { + self.step(now); bump(&tally.packets); // Every draw happens for every datagram, whatever the profile, so one @@ -1140,6 +1244,7 @@ mod tests { const GAUSSIAN: Options = Options { jitter_model: Jitter::Gaussian, batch: None, + steps: Vec::new(), }; #[tokio::test] @@ -1562,4 +1667,224 @@ mod tests { held.up.delayed = 6; assert!(!unbatched(&setup, &held)); } + + /// How long after `at` each of `sizes` leaves one link, fed at the same instant. + fn owed(link: &mut Link, at: Instant, sizes: &[usize]) -> Vec { + let tally = Tally::default(); + sizes + .iter() + .map(|&size| link.treat(at, size, &tally).expect("dropped").0 - at) + .collect() + } + + fn stepped(steps: Vec) -> Options { + Options { + steps, + ..Default::default() + } + } + + #[test] + fn each_step_applies_at_its_own_time() { + let ms = Duration::from_millis; + let profile = Profile { + delay: ms(5), + ..Default::default() + }; + let options = stepped(vec![ + Step { + at: ms(100), + delay: Some(ms(60)), + ..Default::default() + }, + Step { + at: ms(200), + delay: Some(ms(5)), + ..Default::default() + }, + ]); + let (queue, _) = mpsc::unbounded_channel(); + let start = Instant::now(); + let mut link = Link::new(&profile, &options, 7, 0, start, queue); + + assert_eq!(owed(&mut link, start, &[16]), [ms(5)]); + assert_eq!(owed(&mut link, start + ms(100), &[16]), [ms(60)]); + // The step back restores what the run opened with. + assert_eq!(owed(&mut link, start + ms(200), &[16]), [ms(5)]); + } + + #[test] + fn a_loss_step_turns_the_loss_on_and_off_again() { + let secs = Duration::from_secs; + let options = stepped(vec![ + Step { + at: secs(30), + loss: Some(1.0), + ..Default::default() + }, + Step { + at: secs(60), + loss: Some(0.0), + ..Default::default() + }, + ]); + let (queue, _) = mpsc::unbounded_channel(); + let start = Instant::now(); + let mut link = Link::new(&Profile::default(), &options, 7, 0, start, queue); + let tally = Tally::default(); + let mut lost = |at: Instant| (0..100).filter(|_| link.treat(at, 16, &tally).is_none()).count(); + + assert_eq!(lost(start), 0, "the profile opens clean"); + assert_eq!(lost(start + secs(30)), 100, "the step lost nothing"); + assert_eq!(lost(start + secs(60)), 0, "the step back never cleared"); + } + + #[test] + fn a_rate_step_narrows_the_bucket_and_widens_it_again() { + let secs = Duration::from_secs; + let rate = |bytes_per_second: u64| Rate { + bits_per_second: bytes_per_second * 8, + burst: 100, + queue: secs(10), + }; + let profile = Profile { + rate: Some(rate(4000)), + ..Default::default() + }; + let options = stepped(vec![ + Step { + at: secs(30), + rate: Some(rate(1000)), + ..Default::default() + }, + Step { + at: secs(60), + rate: Some(rate(4000)), + ..Default::default() + }, + ]); + let (queue, _) = mpsc::unbounded_channel(); + let start = Instant::now(); + let mut link = Link::new(&profile, &options, 7, 0, start, queue); + + // The bucket is full at each stretch, so the first 100 bytes leave at + // once and the next 100 owe one bucket's refill at the rate in force. + let ms = Duration::from_millis; + assert_eq!(owed(&mut link, start, &[100, 100]), [ms(0), ms(25)]); + assert_eq!(owed(&mut link, start + secs(30), &[100, 100]), [ms(0), ms(100)]); + assert_eq!(owed(&mut link, start + secs(60), &[100, 100]), [ms(0), ms(25)]); + } + + #[test] + fn a_rate_step_keeps_the_debt_it_takes_over() { + let ms = Duration::from_millis; + let rate = |bytes_per_second: u64| Rate { + bits_per_second: bytes_per_second * 8, + burst: 100, + queue: Duration::from_secs(10), + }; + let profile = Profile { + rate: Some(rate(1000)), + ..Default::default() + }; + // Halfway through a 300 byte backlog at 1000 bytes a second, the rate doubles. + let options = stepped(vec![Step { + at: ms(100), + rate: Some(rate(2000)), + ..Default::default() + }]); + let (queue, _) = mpsc::unbounded_channel(); + let start = Instant::now(); + let mut link = Link::new(&profile, &options, 7, 0, start, queue); + assert_eq!(owed(&mut link, start, &[100, 100, 100]), [ms(0), ms(100), ms(200)]); + + // 100 bytes of the backlog are still owed, and now drain in 50ms, so + // a fresh 100 bytes leave once those and themselves are paid. + assert_eq!(owed(&mut link, start + ms(100), &[100]), [ms(100)]); + } + + #[tokio::test] + async fn a_step_gets_worse_part_way_through() { + let ms = Duration::from_millis; + let (_shaper, client) = shaped(|config| Setup { + up: stepped(vec![Step { + at: ms(150), + delay: Some(ms(60)), + ..Default::default() + }]), + ..Config { + up: Profile { + delay: ms(5), + ..Default::default() + }, + ..config + } + .into() + }) + .await; + + let (sent, got) = paced(&client, 40, ms(10)).await; + assert_eq!(got.len(), 40); + let median = |range: std::ops::Range| { + let mut latency: Vec = got[range].iter().map(|&(id, at)| at - sent[id as usize]).collect(); + latency.sort(); + latency[latency.len() / 2] + }; + + // The first ten leave well inside the first 150ms, the last fifteen well after. + let (before, after) = (median(0..10), median(25..40)); + assert!( + after > before + ms(30), + "median latency went from {before:?} to {after:?}" + ); + } + + #[tokio::test] + async fn steps_that_would_be_skipped_or_do_nothing_are_refused() { + let refused = |steps: Vec, why: &'static str| async move { + let config = Config { + bind: LOCALHOST, + target: LOCALHOST, + seed: 0, + up: Profile { + delay: Duration::from_millis(20), + ..Default::default() + }, + down: Profile::default(), + }; + let err = Shaper::bind(Setup { + down: stepped(steps), + ..config.into() + }) + .await + .err() + .unwrap_or_else(|| panic!("accepted steps where {why}")); + assert!(format!("{err:#}").contains(why), "{err:#}"); + }; + let at = |secs| Step { + at: Duration::from_secs(secs), + loss: Some(0.1), + ..Default::default() + }; + + refused(vec![at(60), at(30)], "steps go in order").await; + refused( + vec![Step { + at: Duration::from_secs(30), + ..Default::default() + }], + "changes nothing", + ) + .await; + // Uniform jitter past the delay, reached by a step rather than at the start. + refused( + vec![Step { + at: Duration::from_secs(30), + jitter: Some(Duration::from_millis(10)), + ..Default::default() + }], + "exceeds delay", + ) + .await; + } } diff --git a/rs/moq-shaper/src/main.rs b/rs/moq-shaper/src/main.rs index 6de67ba9e3..6b467067b0 100644 --- a/rs/moq-shaper/src/main.rs +++ b/rs/moq-shaper/src/main.rs @@ -96,6 +96,7 @@ async fn main() -> anyhow::Result<()> { .batch .zip(args.batch_window) .map(|(count, window)| moq_shaper::Batch { count, window }), + ..Default::default() }; let shaper = moq_shaper::Shaper::bind(moq_shaper::Setup { tcp_passthrough: args.tcp_passthrough, From f56f47780f00c1e93f352fb545bcce16816f939e Mon Sep 17 00:00:00 2001 From: fperex <270788582+fperex@users.noreply.github.com> Date: Fri, 25 Sep 2026 07:18:03 -0400 Subject: [PATCH 07/19] feat(shaper): named profiles for both directions, loaded from TOML A run that has to be repeated, compared, or graded against a budget needs its path written down under a name, not rebuilt from flags each time, and the flags cannot say everything anyway: they apply one profile both ways and have no steps. `Preset::load` reads a built-in profile by name, or a TOML file by path, and `Preset::setup` turns it into a `Setup` for an address pair. A file names itself and its seed, says whether the path is shared, and gives each direction one table holding its profile and options: delay, jitter, jitter_model, loss, reorder, rate, batch and steps. Unknown fields are refused, and so is a direction whose delay varies without naming its jitter_model, since whether datagrams can overtake each other then depends on it. The CLI takes `--profile ` in place of the flags that shape the path, with `--seed` still overriding the file's. The built-ins are embedded from profiles/: near-zero, mild, bursty, step and high-rtt, the audio-quality lane's paths, which keep their numbers, their in-order gaussian jitter and their shared path; and lossy, 2% loss with 1% of datagrams skipping a 20ms delay, since a reorder here overtakes by skipping the delay rather than by adding one. `Rate`, `Jitter`, `Batch` and `Step` derive `Deserialize`, and the crate now depends on serde, toml and humantime-serde. Cargo.lock gains only those edges and humantime-serde 1.1.1, the version it had before. Co-Authored-By: Claude Opus 5.5 --- Cargo.lock | 13 ++ rs/moq-shaper/Cargo.toml | 3 + rs/moq-shaper/profiles/bursty.toml | 16 ++ rs/moq-shaper/profiles/high-rtt.toml | 15 ++ rs/moq-shaper/profiles/lossy.toml | 16 ++ rs/moq-shaper/profiles/mild.toml | 15 ++ rs/moq-shaper/profiles/near-zero.toml | 7 + rs/moq-shaper/profiles/step.toml | 24 +++ rs/moq-shaper/src/lib.rs | 25 ++- rs/moq-shaper/src/main.rs | 82 ++++++--- rs/moq-shaper/src/preset.rs | 251 ++++++++++++++++++++++++++ 11 files changed, 438 insertions(+), 29 deletions(-) create mode 100644 rs/moq-shaper/profiles/bursty.toml create mode 100644 rs/moq-shaper/profiles/high-rtt.toml create mode 100644 rs/moq-shaper/profiles/lossy.toml create mode 100644 rs/moq-shaper/profiles/mild.toml create mode 100644 rs/moq-shaper/profiles/near-zero.toml create mode 100644 rs/moq-shaper/profiles/step.toml create mode 100644 rs/moq-shaper/src/preset.rs diff --git a/Cargo.lock b/Cargo.lock index db0d69c052..2ffc9d3146 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3036,6 +3036,16 @@ version = "2.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "15cdd26707701c53297e2fa6afb323d55fbc1d0810c3aec078ae3ef0424c3c15" +[[package]] +name = "humantime-serde" +version = "1.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "57a3db5ea5923d99402c94e9feb261dc5ee9b4efa158b0315f788cf549cc200c" +dependencies = [ + "humantime", + "serde", +] + [[package]] name = "hybrid-array" version = "0.4.15" @@ -4713,8 +4723,11 @@ dependencies = [ "anyhow", "clap", "humantime", + "humantime-serde", "rand 0.10.3", + "serde", "tokio", + "toml 1.1.6+spec-1.1.0", ] [[package]] diff --git a/rs/moq-shaper/Cargo.toml b/rs/moq-shaper/Cargo.toml index ea617a976d..36add548dd 100644 --- a/rs/moq-shaper/Cargo.toml +++ b/rs/moq-shaper/Cargo.toml @@ -20,5 +20,8 @@ doc = false anyhow = { workspace = true } clap = { workspace = true } humantime = { workspace = true } +humantime-serde = { workspace = true } rand = { workspace = true } +serde = { workspace = true } tokio = { workspace = true, features = ["io-util", "macros", "net", "rt-multi-thread", "signal", "sync", "time"] } +toml = { workspace = true } diff --git a/rs/moq-shaper/profiles/bursty.toml b/rs/moq-shaper/profiles/bursty.toml new file mode 100644 index 0000000000..307c8f2f4e --- /dev/null +++ b/rs/moq-shaper/profiles/bursty.toml @@ -0,0 +1,16 @@ +# A paced hop that bunches traffic: nothing moves until seven datagrams are +# waiting or the window closes, and then they all leave together. +# +# This is the shape a receiver's jitter estimate has to see, and no delay-and- +# spread profile produces it: the datagrams arrive together rather than late. +name = "bursty" +seed = 3 +shared = true + +[up.batch] +count = 7 +window = "160ms" + +[down.batch] +count = 7 +window = "160ms" diff --git a/rs/moq-shaper/profiles/high-rtt.toml b/rs/moq-shaper/profiles/high-rtt.toml new file mode 100644 index 0000000000..1f600fbbf3 --- /dev/null +++ b/rs/moq-shaper/profiles/high-rtt.toml @@ -0,0 +1,15 @@ +# An intercontinental path: 75ms each way with a wide spread on top, queued in +# order. +name = "high-rtt" +seed = 5 +shared = true + +[up] +delay = "75ms" +jitter = "30ms" +jitter_model = "gaussian" + +[down] +delay = "75ms" +jitter = "30ms" +jitter_model = "gaussian" diff --git a/rs/moq-shaper/profiles/lossy.toml b/rs/moq-shaper/profiles/lossy.toml new file mode 100644 index 0000000000..816063a752 --- /dev/null +++ b/rs/moq-shaper/profiles/lossy.toml @@ -0,0 +1,16 @@ +# A path that loses 2%, and on which 1% of what survives skips the 20ms delay +# the rest take, arriving ahead of what is still in flight: the loss and the +# reordering a recovery path has to cope with. +name = "lossy" +seed = 6 +shared = true + +[up] +delay = "20ms" +loss = 0.02 +reorder = 0.01 + +[down] +delay = "20ms" +loss = 0.02 +reorder = 0.01 diff --git a/rs/moq-shaper/profiles/mild.toml b/rs/moq-shaper/profiles/mild.toml new file mode 100644 index 0000000000..de101bb5bf --- /dev/null +++ b/rs/moq-shaper/profiles/mild.toml @@ -0,0 +1,15 @@ +# A healthy wired LAN: a few milliseconds of delay with the same spread again, +# queued in order. +name = "mild" +seed = 2 +shared = true + +[up] +delay = "5ms" +jitter = "5ms" +jitter_model = "gaussian" + +[down] +delay = "5ms" +jitter = "5ms" +jitter_model = "gaussian" diff --git a/rs/moq-shaper/profiles/near-zero.toml b/rs/moq-shaper/profiles/near-zero.toml new file mode 100644 index 0000000000..627f619487 --- /dev/null +++ b/rs/moq-shaper/profiles/near-zero.toml @@ -0,0 +1,7 @@ +# The control: the shaper sits in the path but treats nothing. +# +# Every other profile is graded against this one, and a harness asserting that an +# impairment applied needs a profile where `delayed == 0` is the right answer. +name = "near-zero" +seed = 1 +shared = true diff --git a/rs/moq-shaper/profiles/step.toml b/rs/moq-shaper/profiles/step.toml new file mode 100644 index 0000000000..bdd0170d2f --- /dev/null +++ b/rs/moq-shaper/profiles/step.toml @@ -0,0 +1,24 @@ +# A path that gets worse mid-run: 5ms for the first thirty seconds, then 60ms. +# +# An estimator that only ever converges downward passes every steady profile and +# fails this one. It never recovers, which is what makes it the baseline a +# recovery profile is read against. +name = "step" +seed = 4 +shared = true + +[up] +delay = "5ms" +jitter_model = "gaussian" + +[[up.steps]] +at = "30s" +delay = "60ms" + +[down] +delay = "5ms" +jitter_model = "gaussian" + +[[down.steps]] +at = "30s" +delay = "60ms" diff --git a/rs/moq-shaper/src/lib.rs b/rs/moq-shaper/src/lib.rs index 90b01f777e..c3c471abdd 100644 --- a/rs/moq-shaper/src/lib.rs +++ b/rs/moq-shaper/src/lib.rs @@ -26,8 +26,13 @@ use std::{ time::Duration, }; +mod preset; + +pub use preset::Preset; + use anyhow::Context; use rand::{RngExt, SeedableRng, rngs::Xoshiro256PlusPlus}; +use serde::Deserialize; use tokio::{ net::{TcpListener, TcpStream, UdpSocket}, sync::mpsc, @@ -87,7 +92,8 @@ impl Profile { } /// How a direction draws each datagram's jitter. -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Deserialize)] +#[serde(rename_all = "lowercase")] pub enum Jitter { /// Uniform within `jitter` either way of `delay`, drawn for each datagram on /// its own, so a later datagram can overtake an earlier one. @@ -108,11 +114,13 @@ pub enum Jitter { /// first of them arrived, and everything in it leaves when the latest would. /// That clump is what a receiver's jitter estimate sees on such a path, and no /// delay or jitter produces it: the datagrams arrive together rather than late. -#[derive(Clone, Copy, Debug, PartialEq, Eq)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Deserialize)] +#[serde(deny_unknown_fields)] pub struct Batch { /// How many datagrams close a batch early. pub count: usize, /// How long a batch waits for that many. + #[serde(with = "humantime_serde")] pub window: Duration, } @@ -121,19 +129,26 @@ pub struct Batch { /// A step changes only what it names, so a later step puts one knob back /// without restating the rest. It can add or change a rate limit, never remove /// one. -#[derive(Clone, Debug, Default, PartialEq)] +#[derive(Clone, Debug, Default, PartialEq, Deserialize)] +#[serde(deny_unknown_fields)] pub struct Step { /// How far into the run the change happens. + #[serde(with = "humantime_serde")] pub at: Duration, /// The delay from then on. + #[serde(default, with = "humantime_serde")] pub delay: Option, /// The jitter from then on. + #[serde(default, with = "humantime_serde")] pub jitter: Option, /// The loss from then on. + #[serde(default)] pub loss: Option, /// The reorder from then on. + #[serde(default)] pub reorder: Option, /// The rate limit from then on. + #[serde(default)] pub rate: Option, } @@ -254,13 +269,15 @@ fn unbatched(setup: &Setup, stats: &Stats) -> bool { } /// A token-bucket rate limit with a bounded queue behind it. -#[derive(Clone, Debug, PartialEq)] +#[derive(Clone, Debug, PartialEq, Deserialize)] +#[serde(deny_unknown_fields)] pub struct Rate { /// The sustained rate. pub bits_per_second: u64, /// How many bytes may go out ahead of the sustained rate after an idle spell. pub burst: u64, /// The longest a datagram waits for the bucket; one that would wait longer is dropped. + #[serde(with = "humantime_serde")] pub queue: Duration, } diff --git a/rs/moq-shaper/src/main.rs b/rs/moq-shaper/src/main.rs index 6b467067b0..d05c07c323 100644 --- a/rs/moq-shaper/src/main.rs +++ b/rs/moq-shaper/src/main.rs @@ -1,7 +1,9 @@ //! Put a seeded, impaired UDP path in front of a server. //! -//! Both directions get the same profile. The seed and profile print at start, -//! and the counters at exit, which fails if the profile never acted. +//! Both directions get the same profile from the flags, unless `--profile` +//! names a profile file, which sets each direction on its own. The seed and +//! profile print at start, and the counters at exit, which fails if the profile +//! never acted. use std::{net::SocketAddr, time::Duration}; @@ -16,9 +18,19 @@ struct Args { /// The address every datagram is forwarded to. #[arg(long)] target: SocketAddr, - /// Seeds every treatment decision; random when omitted. + /// Seeds every treatment decision; the profile's own, or random, when omitted. #[arg(long)] seed: Option, + /// A built-in profile's name, or a profile TOML file, in place of the flags + /// that shape the path. + #[arg( + long, + conflicts_with_all = [ + "delay", "jitter", "jitter_model", "loss", "reorder", "rate", "burst", "queue", "batch", "batch_window", + "shared", + ], + )] + profile: Option, /// The base one-way delay, e.g. `20ms`. #[arg(long, default_value = "0s", value_parser = humantime::parse_duration)] delay: Duration, @@ -69,6 +81,46 @@ enum JitterModel { async fn main() -> anyhow::Result<()> { let args = Args::parse(); + let (name, mut setup) = match &args.profile { + Some(profile) => { + let preset = moq_shaper::Preset::load(profile)?; + let mut setup = preset.setup(args.listen, args.target); + if let Some(seed) = args.seed { + setup.config.seed = seed; + } + (Some(preset.name), setup) + } + None => (None, flags(&args)), + }; + setup.tcp_passthrough = args.tcp_passthrough; + + let treatment = match &name { + Some(name) => format!("profile {name}"), + None if setup.shared || setup.up != moq_shaper::Options::default() => format!( + "profile {:?}, options {:?}, shared {}", + setup.config.up, setup.up, setup.shared + ), + None => format!("profile {:?}", setup.config.up), + }; + let shaper = moq_shaper::Shaper::bind(setup).await?; + + let config = shaper.config(); + println!( + "shaper: {} -> {}, seed {}, {treatment}", + shaper.addr(), + config.target, + config.seed + ); + + shutdown().await?; + + let stats = shaper.verify()?; + println!("shaper: {stats}"); + Ok(()) +} + +/// The setup the flags describe: one profile and one set of options, both ways. +fn flags(args: &Args) -> moq_shaper::Setup { let profile = moq_shaper::Profile { delay: args.delay, jitter: args.jitter, @@ -98,32 +150,12 @@ async fn main() -> anyhow::Result<()> { .map(|(count, window)| moq_shaper::Batch { count, window }), ..Default::default() }; - let shaper = moq_shaper::Shaper::bind(moq_shaper::Setup { - tcp_passthrough: args.tcp_passthrough, + moq_shaper::Setup { shared: args.shared, up: options.clone(), - down: options.clone(), + down: options, ..config.into() - }) - .await?; - - let config = shaper.config(); - println!( - "shaper: {} -> {}, seed {}, profile {:?}", - shaper.addr(), - config.target, - config.seed, - config.up - ); - if options != moq_shaper::Options::default() || args.shared { - println!("shaper: options {options:?}, shared {}", args.shared); } - - shutdown().await?; - - let stats = shaper.verify()?; - println!("shaper: {stats}"); - Ok(()) } /// Wait for Ctrl-C, or SIGTERM where there is one, which is how a harness stops a child. diff --git a/rs/moq-shaper/src/preset.rs b/rs/moq-shaper/src/preset.rs new file mode 100644 index 0000000000..56c01a1778 --- /dev/null +++ b/rs/moq-shaper/src/preset.rs @@ -0,0 +1,251 @@ +use std::{net::SocketAddr, time::Duration}; + +use anyhow::Context; +use serde::Deserialize; + +use crate::{Batch, Config, Jitter, Options, Profile, Rate, Setup, Step}; + +/// The profiles shipped under `profiles/`, embedded so a binary needs no data files. +const BUILTIN: &[(&str, &str)] = &[ + ("near-zero", include_str!("../profiles/near-zero.toml")), + ("mild", include_str!("../profiles/mild.toml")), + ("bursty", include_str!("../profiles/bursty.toml")), + ("step", include_str!("../profiles/step.toml")), + ("high-rtt", include_str!("../profiles/high-rtt.toml")), + ("lossy", include_str!("../profiles/lossy.toml")), +]; + +/// A named, seeded treatment of both directions, loaded from a TOML profile. +#[derive(Clone, Debug)] +pub struct Preset { + /// The name a run records its counters under. + pub name: String, + seed: u64, + shared: bool, + up: (Profile, Options), + down: (Profile, Options), +} + +impl Preset { + /// Load a built-in profile by name, or a profile file by path. + pub fn load(name_or_path: &str) -> anyhow::Result { + let text = match BUILTIN.iter().find(|(name, _)| *name == name_or_path) { + Some((_, text)) => (*text).to_string(), + // An unknown name is almost always a typo, so say what the choices were. + None => std::fs::read_to_string(name_or_path).with_context(|| { + format!( + "no built-in profile or readable file named {name_or_path:?}; the built-ins are {}", + Self::names().collect::>().join(", ") + ) + })?, + }; + + let file: File = toml::from_str(&text).with_context(|| format!("parse profile {name_or_path:?}"))?; + file.preset() + .with_context(|| format!("invalid profile {name_or_path:?}")) + } + + /// The built-in profiles' names. + pub fn names() -> impl Iterator { + BUILTIN.iter().map(|(name, _)| *name) + } + + /// This preset listening on `bind` and forwarding to `target`, with its own seed. + pub fn setup(&self, bind: SocketAddr, target: SocketAddr) -> Setup { + Setup { + config: Config { + bind, + target, + seed: self.seed, + up: self.up.0.clone(), + down: self.down.0.clone(), + }, + tcp_passthrough: false, + shared: self.shared, + up: self.up.1.clone(), + down: self.down.1.clone(), + } + } +} + +/// A profile file. +#[derive(Deserialize)] +#[serde(deny_unknown_fields)] +struct File { + name: String, + seed: u64, + #[serde(default)] + shared: bool, + #[serde(default)] + up: Side, + #[serde(default)] + down: Side, +} + +impl File { + fn preset(self) -> anyhow::Result { + anyhow::ensure!(!self.name.is_empty(), "the profile has no name"); + Ok(Preset { + up: self.up.split().context("up")?, + down: self.down.split().context("down")?, + name: self.name, + seed: self.seed, + shared: self.shared, + }) + } +} + +/// One direction of a profile file: its [`Profile`] and [`Options`] in one table. +#[derive(Default, Deserialize)] +#[serde(default, deny_unknown_fields)] +struct Side { + #[serde(with = "humantime_serde")] + delay: Duration, + #[serde(with = "humantime_serde")] + jitter: Duration, + jitter_model: Option, + loss: f64, + reorder: f64, + rate: Option, + batch: Option, + steps: Vec, +} + +impl Side { + fn split(self) -> anyhow::Result<(Profile, Options)> { + // Whether datagrams can overtake each other depends on the model as soon + // as their delays differ, so a file has to say which it means rather + // than inherit whichever is the default. + let varies = !self.jitter.is_zero() + || self + .steps + .iter() + .any(|step| step.delay.is_some() || step.jitter.is_some_and(|jitter| !jitter.is_zero())); + anyhow::ensure!( + self.jitter_model.is_some() || !varies, + "the delay varies, so name a jitter_model: \"uniform\" lets a datagram overtake the one in front, \"gaussian\" never does" + ); + + let profile = Profile { + delay: self.delay, + jitter: self.jitter, + loss: self.loss, + reorder: self.reorder, + rate: self.rate, + }; + let options = Options { + jitter_model: self.jitter_model.unwrap_or_default(), + batch: self.batch, + steps: self.steps, + }; + options.validate(&profile)?; + Ok((profile, options)) + } +} + +#[cfg(test)] +mod tests { + use std::net::{IpAddr, Ipv4Addr}; + + use super::*; + + const LOCALHOST: SocketAddr = SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 0); + + #[test] + fn builtins_parse_and_are_named_after_their_file() { + for name in Preset::names() { + let preset = Preset::load(name).unwrap_or_else(|err| panic!("{name}: {err:#}")); + assert_eq!(preset.name, name); + } + } + + #[test] + fn builtins_treat_the_path_except_the_control() { + for name in Preset::names() { + let preset = Preset::load(name).unwrap(); + let untreated = (Profile::default(), Options::default()); + let idle = preset.up == untreated && preset.down == untreated; + assert_eq!(idle, name == "near-zero", "{name} treats the wrong amount of traffic"); + } + } + + #[test] + fn the_step_builtin_steps_both_directions_at_thirty_seconds() { + let setup = Preset::load("step").unwrap().setup(LOCALHOST, LOCALHOST); + for (which, profile, options) in [ + ("up", &setup.config.up, &setup.up), + ("down", &setup.config.down, &setup.down), + ] { + assert_eq!(profile.delay, Duration::from_millis(5), "{which}"); + assert_eq!(options.jitter_model, Jitter::Gaussian, "{which}"); + assert_eq!(options.steps.len(), 1, "{which}"); + assert_eq!(options.steps[0].at, Duration::from_secs(30), "{which}"); + assert_eq!(options.steps[0].delay, Some(Duration::from_millis(60)), "{which}"); + } + } + + #[test] + fn an_unknown_name_lists_the_builtins() { + let err = format!("{:#}", Preset::load("nope").unwrap_err()); + assert!(err.contains("near-zero"), "{err}"); + } + + #[test] + fn an_unknown_field_is_refused() { + // A field the schema does not know, like a rate in bytes, is refused rather than ignored. + let Err(err) = toml::from_str::("name = \"x\"\nseed = 1\n[up.rate]\nbytes_per_second = 1000\n") else { + panic!("a rate in bytes parsed"); + }; + assert!(err.to_string().contains("bytes_per_second"), "{err}"); + } + + #[test] + fn a_varying_delay_has_to_name_its_model() { + let parse = |text: &str| toml::from_str::(text).unwrap().preset(); + + let err = parse("name = \"x\"\nseed = 1\n[up]\ndelay = \"5ms\"\njitter = \"5ms\"\n").unwrap_err(); + assert!(format!("{err:#}").contains("jitter_model"), "{err:#}"); + + let err = parse("name = \"x\"\nseed = 1\n[up]\ndelay = \"5ms\"\n[[up.steps]]\nat = \"1s\"\ndelay = \"9ms\"\n") + .unwrap_err(); + assert!(format!("{err:#}").contains("jitter_model"), "{err:#}"); + + // A fixed delay cannot reorder anything, so it needs no model. + parse("name = \"x\"\nseed = 1\n[up]\ndelay = \"5ms\"\n").unwrap(); + parse("name = \"x\"\nseed = 1\n[up]\ndelay = \"5ms\"\njitter = \"5ms\"\njitter_model = \"uniform\"\n").unwrap(); + } + + #[test] + fn a_profile_file_loads_by_path() { + let path = std::env::temp_dir().join(format!("moq-shaper-preset-{}.toml", std::process::id())); + std::fs::write( + &path, + "name = \"file\"\nseed = 9\n[down.batch]\ncount = 3\nwindow = \"50ms\"\n", + ) + .unwrap(); + let preset = Preset::load(path.to_str().unwrap()); + std::fs::remove_file(&path).unwrap(); + + let setup = preset.unwrap().setup(LOCALHOST, LOCALHOST); + assert_eq!(setup.config.seed, 9); + assert_eq!( + setup.down.batch, + Some(Batch { + count: 3, + window: Duration::from_millis(50) + }) + ); + } + + #[tokio::test] + async fn the_builtins_all_bind() { + for name in Preset::names() { + // Binding proves the profile is usable, not just parseable. + let preset = Preset::load(name).unwrap(); + let shaper = crate::Shaper::bind(preset.setup(LOCALHOST, LOCALHOST)) + .await + .unwrap_or_else(|err| panic!("{name}: {err:#}")); + shaper.verify().unwrap(); + } + } +} From c630613c9a59d04e00ae39f0ebc7bd9b2e03df44 Mon Sep 17 00:00:00 2001 From: fperex <270788582+fperex@users.noreply.github.com> Date: Fri, 25 Sep 2026 07:19:31 -0400 Subject: [PATCH 08/19] feat(shaper): a JSON report of the counters, as an option A harness grading a run needs the shaper's counters in a form it can read, and a run that proved nothing (a profile that never acted) has to be told apart from one that passed. The exit line is prose, and it is printed only after the verdict, so a failed run leaves nothing to read. `Counters` and `Stats` derive `Serialize`. The CLI's `--report ` writes `{"profile", "seed", "up", "down"}` at exit, before the verdict, so a run that fails it still leaves its counters behind; `profile` is the profile's name, or null when the flags built it. `--report-interval` prints the same object as one line on stdout each period, which also survives a shaper killed before it could write the file. Off by default. Co-Authored-By: Claude Opus 5.5 --- Cargo.lock | 1 + rs/moq-shaper/Cargo.toml | 1 + rs/moq-shaper/src/lib.rs | 6 +-- rs/moq-shaper/src/main.rs | 56 ++++++++++++++++++++++++- rs/moq-shaper/tests/cli.rs | 83 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 142 insertions(+), 5 deletions(-) create mode 100644 rs/moq-shaper/tests/cli.rs diff --git a/Cargo.lock b/Cargo.lock index 2ffc9d3146..7273dc240f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4726,6 +4726,7 @@ dependencies = [ "humantime-serde", "rand 0.10.3", "serde", + "serde_json", "tokio", "toml 1.1.6+spec-1.1.0", ] diff --git a/rs/moq-shaper/Cargo.toml b/rs/moq-shaper/Cargo.toml index 36add548dd..f81759d5e1 100644 --- a/rs/moq-shaper/Cargo.toml +++ b/rs/moq-shaper/Cargo.toml @@ -23,5 +23,6 @@ humantime = { workspace = true } humantime-serde = { workspace = true } rand = { workspace = true } serde = { workspace = true } +serde_json = { workspace = true } tokio = { workspace = true, features = ["io-util", "macros", "net", "rt-multi-thread", "signal", "sync", "time"] } toml = { workspace = true } diff --git a/rs/moq-shaper/src/lib.rs b/rs/moq-shaper/src/lib.rs index c3c471abdd..073dcb5c79 100644 --- a/rs/moq-shaper/src/lib.rs +++ b/rs/moq-shaper/src/lib.rs @@ -32,7 +32,7 @@ pub use preset::Preset; use anyhow::Context; use rand::{RngExt, SeedableRng, rngs::Xoshiro256PlusPlus}; -use serde::Deserialize; +use serde::{Deserialize, Serialize}; use tokio::{ net::{TcpListener, TcpStream, UdpSocket}, sync::mpsc, @@ -344,7 +344,7 @@ impl From for Setup { } /// What one direction did, summed over every client. -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize)] pub struct Counters { /// Datagrams received. pub packets: u64, @@ -371,7 +371,7 @@ impl fmt::Display for Counters { } /// What both directions did. -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize)] pub struct Stats { /// From clients toward the target. pub up: Counters, diff --git a/rs/moq-shaper/src/main.rs b/rs/moq-shaper/src/main.rs index d05c07c323..0fac2dacd7 100644 --- a/rs/moq-shaper/src/main.rs +++ b/rs/moq-shaper/src/main.rs @@ -5,8 +5,9 @@ //! profile print at start, and the counters at exit, which fails if the profile //! never acted. -use std::{net::SocketAddr, time::Duration}; +use std::{net::SocketAddr, path::PathBuf, time::Duration}; +use anyhow::Context; use clap::Parser; #[derive(Parser)] @@ -67,6 +68,12 @@ struct Args { /// Also pipe TCP on the listening port to the target, untouched. #[arg(long)] tcp_passthrough: bool, + /// Write the profile, seed and counters to this file as JSON at exit. + #[arg(long)] + report: Option, + /// Print the same JSON as one line on stdout this often. + #[arg(long, value_parser = humantime::parse_duration)] + report_interval: Option, } #[derive(Clone, Copy, clap::ValueEnum)] @@ -112,7 +119,32 @@ async fn main() -> anyhow::Result<()> { config.seed ); - shutdown().await?; + let report = || Report { + profile: name.as_deref(), + seed: config.seed, + stats: shaper.stats(), + }; + + // The first tick is a full period in, so a line reports traffic, not the start. + let mut interval = args + .report_interval + .map(|period| tokio::time::interval_at(tokio::time::Instant::now() + period, period)); + let shutdown = shutdown(); + tokio::pin!(shutdown); + loop { + tokio::select! { + res = &mut shutdown => { + res?; + break; + } + _ = tick(&mut interval) => println!("{}", serde_json::to_string(&report())?), + } + } + + // Before the verdict, so a run that fails it still leaves its counters behind. + if let Some(path) = &args.report { + std::fs::write(path, serde_json::to_vec(&report())?).with_context(|| format!("write {}", path.display()))?; + } let stats = shaper.verify()?; println!("shaper: {stats}"); @@ -158,6 +190,26 @@ fn flags(args: &Args) -> moq_shaper::Setup { } } +/// What a run did, as the JSON a harness reads back. +#[derive(serde::Serialize)] +struct Report<'a> { + /// The profile's name, or null when the flags built it. + profile: Option<&'a str>, + seed: u64, + #[serde(flatten)] + stats: moq_shaper::Stats, +} + +/// The next report tick, or never without an interval. +async fn tick(interval: &mut Option) { + match interval { + Some(interval) => { + interval.tick().await; + } + None => std::future::pending().await, + } +} + /// Wait for Ctrl-C, or SIGTERM where there is one, which is how a harness stops a child. async fn shutdown() -> anyhow::Result<()> { #[cfg(unix)] diff --git a/rs/moq-shaper/tests/cli.rs b/rs/moq-shaper/tests/cli.rs new file mode 100644 index 0000000000..fb37ba9d1f --- /dev/null +++ b/rs/moq-shaper/tests/cli.rs @@ -0,0 +1,83 @@ +//! The binary end to end: a profile by name, the TCP passthrough, and the +//! counters it reports while it runs and when it stops. +#![cfg(unix)] + +use std::{ + io::{BufRead, BufReader, Read, Write}, + net::{SocketAddr, TcpListener, TcpStream, UdpSocket}, + process::{Command, Stdio}, + time::Duration, +}; + +#[test] +fn a_profile_run_reports_its_counters() { + // The target answers TCP on the port number it takes datagrams on, as a relay does. + let target = UdpSocket::bind("127.0.0.1:0").unwrap(); + let target_addr = target.local_addr().unwrap(); + let http = TcpListener::bind(target_addr).unwrap(); + std::thread::spawn(move || { + let (mut stream, _) = http.accept().unwrap(); + let mut buf = [0u8; 64]; + let size = stream.read(&mut buf).unwrap(); + stream.write_all(&buf[..size]).unwrap(); + }); + + let report = std::env::temp_dir().join(format!("moq-shaper-report-{}.json", std::process::id())); + let mut shaper = Command::new(env!("CARGO_BIN_EXE_moq-shaper")) + .args(["--listen", "127.0.0.1:0", "--target", &target_addr.to_string()]) + .args(["--profile", "mild", "--seed", "7", "--tcp-passthrough"]) + .args(["--report-interval", "100ms", "--report", report.to_str().unwrap()]) + .stdout(Stdio::piped()) + .spawn() + .unwrap(); + let mut stdout = BufReader::new(shaper.stdout.take().unwrap()); + + // The first line says where it listens, and under what. + let mut line = String::new(); + stdout.read_line(&mut line).unwrap(); + assert!(line.ends_with("seed 7, profile mild\n"), "{line}"); + let addr: SocketAddr = line.split_whitespace().nth(1).unwrap().parse().unwrap(); + + let client = UdpSocket::bind("127.0.0.1:0").unwrap(); + for id in 0..50u32 { + client.send_to(&id.to_be_bytes(), addr).unwrap(); + } + target.set_read_timeout(Some(Duration::from_secs(5))).unwrap(); + let mut buf = [0u8; 4]; + for _ in 0..50 { + target.recv_from(&mut buf).expect("a datagram never arrived"); + } + + let mut tcp = TcpStream::connect(addr).unwrap(); + tcp.write_all(b"/certificate.sha256").unwrap(); + let mut buf = [0u8; 64]; + let size = tcp.read(&mut buf).unwrap(); + assert_eq!(&buf[..size], b"/certificate.sha256"); + + // A line each interval, once the traffic has been counted. + let interim = loop { + line.clear(); + stdout.read_line(&mut line).unwrap(); + let json: serde_json::Value = serde_json::from_str(&line).unwrap(); + if json["up"]["packets"] == 50 { + break json; + } + }; + assert_eq!(interim["profile"], "mild"); + + let status = Command::new("kill") + .args(["-TERM", &shaper.id().to_string()]) + .status() + .unwrap(); + assert!(status.success()); + assert!(shaper.wait().unwrap().success(), "the profile never acted"); + + let written = std::fs::read(&report).expect("no report at exit"); + std::fs::remove_file(&report).unwrap(); + let json: serde_json::Value = serde_json::from_slice(&written).unwrap(); + assert_eq!(json["profile"], "mild"); + assert_eq!(json["seed"], 7); + assert_eq!(json["up"]["packets"], 50); + assert_eq!(json["up"]["delayed"], 50); + assert_eq!(json["up"]["lost"], 0); +} From f564161e2ed49a252b190aa382383b737b8b61b9 Mon Sep 17 00:00:00 2001 From: fperex <270788582+fperex@users.noreply.github.com> Date: Fri, 25 Sep 2026 07:25:42 -0400 Subject: [PATCH 09/19] fix(shaper): send a datagram that is already due at once, not on a timer The delivery task sent datagrams only when its timer fired, even ones whose departure had already come. tokio rounds a deadline up to the end of its millisecond, so every datagram waited for the next tick: on loopback with an empty profile, the shaper added 1.5ms each way where a forward takes 0.3ms. Every profile carried that on top of its own delay, so the control profile was not a control, and a batch that filled left a tick late. After each datagram arrives, and each time the timer fires, whatever is due now leaves now. Nothing leaves before its departure, and the timer still wakes for the ones that are not due yet. A paused-clock test pins it: an undelayed round trip takes no time, where before the fix it took 2ms, a tick each way. Co-Authored-By: Claude Opus 5.5 --- rs/moq-shaper/Cargo.toml | 3 +++ rs/moq-shaper/src/lib.rs | 45 ++++++++++++++++++++++++++++------------ 2 files changed, 35 insertions(+), 13 deletions(-) diff --git a/rs/moq-shaper/Cargo.toml b/rs/moq-shaper/Cargo.toml index f81759d5e1..a18c3cc8fc 100644 --- a/rs/moq-shaper/Cargo.toml +++ b/rs/moq-shaper/Cargo.toml @@ -26,3 +26,6 @@ serde = { workspace = true } serde_json = { workspace = true } tokio = { workspace = true, features = ["io-util", "macros", "net", "rt-multi-thread", "signal", "sync", "time"] } toml = { workspace = true } + +[dev-dependencies] +tokio = { workspace = true, features = ["test-util"] } diff --git a/rs/moq-shaper/src/lib.rs b/rs/moq-shaper/src/lib.rs index 073dcb5c79..7d0f17d99a 100644 --- a/rs/moq-shaper/src/lib.rs +++ b/rs/moq-shaper/src/lib.rs @@ -699,20 +699,8 @@ async fn deliver( } } _ = tokio::time::sleep_until(wake.unwrap_or_else(Instant::now)), if wake.is_some() => { - let now = Instant::now(); - while let Some(&Reverse((at, id))) = pending.peek() { - if at > now { - break; - } - pending.pop(); - let parcel = parcels.remove(&id).expect("queued datagram"); - // A send error is the path losing the datagram, the way a - // network does when the far end is gone (a killed relay, say), - // so it is not the shaper failing. - let _ = parcel.socket.send_to(&parcel.datagram, parcel.dest).await; - } // The window closes a batch that never filled. - if let Some(closes) = held.closes.filter(|&closes| closes <= now) { + if let Some(closes) = held.closes.filter(|&closes| closes <= Instant::now()) { held.release(closes, &mut ready, &tally[direction]); } } @@ -722,6 +710,21 @@ async fn deliver( parcels.insert(sequence, parcel); sequence += 1; } + + // Whatever is due leaves now. The timer rounds up to the next + // millisecond, which would delay even a datagram given no delay. + let now = Instant::now(); + while let Some(&Reverse((at, id))) = pending.peek() { + if at > now { + break; + } + pending.pop(); + let parcel = parcels.remove(&id).expect("queued datagram"); + // A send error is the path losing the datagram, the way a network + // does when the far end is gone (a killed relay, say), so it is not + // the shaper failing. + let _ = parcel.socket.send_to(&parcel.datagram, parcel.dest).await; + } } } @@ -988,6 +991,22 @@ mod tests { assert_eq!(stats.down.packets, 50); } + #[tokio::test(start_paused = true)] + async fn an_undelayed_datagram_waits_for_no_timer() { + let (_shaper, client) = setup(1, Profile::default(), Profile::default()).await; + + // Off a millisecond boundary, where a timer would round up to the next one. + tokio::time::advance(Duration::from_micros(500)).await; + let start = Instant::now(); + client.send(b"ping").await.unwrap(); + let mut buf = [0u8; 4]; + client.recv(&mut buf).await.unwrap(); + + // A paused clock only moves when every task waits on a timer, so a round + // trip that took any time slept on one. + assert_eq!(start.elapsed(), Duration::ZERO); + } + #[tokio::test] async fn the_seed_reproduces_the_losses() { let lossy = Profile { From 78046628c78a4a07095c0f1c4e4bbec7e2cd661a Mon Sep 17 00:00:00 2001 From: fperex <270788582+fperex@users.noreply.github.com> Date: Fri, 25 Sep 2026 07:29:18 -0400 Subject: [PATCH 10/19] docs(shaper): a README for the shaper's usage and its options The crate had no README, and its options are only worth taking with the reason behind each: why jitter should keep the order, why a page's sessions share one path, why a batch is not a delay, why TCP passes through untouched. The README covers upstream's usage and flags, what a seed fixes and what it does not, each option, the profile file format and the built-ins, the counters, and the report. The crate docs point at the options. Co-Authored-By: Claude Opus 5.5 --- rs/moq-shaper/README.md | 179 +++++++++++++++++++++++++++++++++++++++ rs/moq-shaper/src/lib.rs | 4 + 2 files changed, 183 insertions(+) create mode 100644 rs/moq-shaper/README.md diff --git a/rs/moq-shaper/README.md b/rs/moq-shaper/README.md new file mode 100644 index 0000000000..8eb817054a --- /dev/null +++ b/rs/moq-shaper/README.md @@ -0,0 +1,179 @@ +# moq-shaper + +A seeded userspace UDP impairment relay, so a test can put an impaired path in front of a server. + +The shaper binds one socket, forwards every datagram to a target after a profile's treatment, and +forwards the replies back. A client dials the shaper instead of the server and reaches it through a +path with loss, a rate limit, delay, jitter, and reordering. QUIC is indifferent to the extra hop, so +this impairs a real transport with no capabilities and nothing touching the host's network, on macOS +and Linux alike. Kernel impairment (`netem` in a network namespace) is Linux only, needs +`CAP_NET_ADMIN`, and changes the host's network for everything running on it. + +It is a datagram relay, not an HTTP interceptor or a TCP proxy, neither of which can impair QUIC. + +## Usage + +```bash +moq-relay --listen 127.0.0.1:4443 & + +moq-shaper --listen 127.0.0.1:4444 --target 127.0.0.1:4443 \ + --delay 20ms --jitter 5ms --loss 0.05 --reorder 0.02 --rate 10000000 --seed 1 +``` + +Point the client at the shaper's port. The flags build one profile, applied both ways. The seed and +profile print at start, and the counters at exit (Ctrl-C or SIGTERM), which exits nonzero if the +profile never acted. + +| Flag | What it does | +| --- | --- | +| `--listen` | The address clients send to. | +| `--target` | The address every datagram is forwarded to. | +| `--seed` | Seeds every decision; the profile's own, or random, when omitted. | +| `--delay` | The base one-way delay. | +| `--jitter` | How far the delay varies: the most either way, or the sigma of a gaussian. | +| `--jitter-model` | `uniform` (the default) or `gaussian`. See [Jitter](#jitter). | +| `--loss` | The probability that a datagram is dropped. | +| `--reorder` | The probability that a datagram skips the delay, overtaking those in flight. | +| `--rate`, `--burst`, `--queue` | A token bucket: bits per second, the bytes it lets out ahead after an idle spell (1500), and the longest a datagram waits before it is dropped (100ms). | +| `--batch`, `--batch-window` | Hold datagrams until this many wait, or the window closes, then release them together. See [Batches](#batches). | +| `--shared` | Every client shares one link each way. See [Shared path](#shared-path). | +| `--profile` | A built-in profile's name, or a profile TOML file, in place of the flags that shape the path. See [Profiles](#profiles). | +| `--tcp-passthrough` | Also pipe TCP on the listening port to the target, untouched. See [TCP](#tcp). | +| `--report` | Write the profile, seed and counters to this file as JSON at exit. See [Report](#report). | +| `--report-interval` | Print the same JSON as one line on stdout this often. | + +As a library, `Shaper::bind` takes a `Config`: where to listen and forward, the seed, and a `Profile` +per direction. It also takes a `Setup`, which is a `Config` plus the opt-in options below, each off by +default. `Shaper::verify` fails when the shaper stopped forwarding, or when an impairment the profile +configures never acted and the traffic makes that silence implausible. The relay's drills +(`rs/moq-relay/tests/drills.rs`, described in `test/drill/README.md`) run every scenario through it. + +## What a seed does and does not fix + +Every decision comes from the seed: each link draws loss, reorder and jitter from its own stream, in the +same order for every datagram whether or not the knob is on. Replay a run with the same seed and the +same datagrams are dropped, the same ones skip the delay, and each draws the same jitter. + +The seed does not make the kernel's delivery clock reproducible: when a held datagram actually lands is +the host's scheduling. A test asserting on a replay should assert on the decisions, not on arrival times. + +## Jitter + +The default, `uniform`, draws each datagram's delay on its own within `jitter` either way of `delay`, so a +later datagram can overtake an earlier one. A drill wants that, and `jitter` may not exceed `delay`. + +`gaussian` treats `jitter` as a sigma, clamps the draw at zero, and never lets a datagram leave before the +one in front of it: queueing delay on a FIFO path, which stretches and compresses the spacing and never +changes the order. Only `reorder` overtakes. This matters beyond realism: QUIC reads an overtake as loss, +retransmits and backs off, so a run meant to measure a jittery path ends up measuring the congestion +response. On a 5ms delay with 5ms of jitter, reordering alone dragged a receiver's audio buffer from +120ms to nearly two seconds. + +## Shared path + +Each client flow gets a link of its own each way by default: its own seeded stream, rate limit and queue, +so its decisions do not depend on anyone else's traffic. + +With `shared`, every flow takes one link each way, the way clients behind one access link do. A browser +page with a WebTransport session of its own and another in its audio worker is two flows on one path: +shared, a gaussian path keeps the order across both, a batch fills from both, and one rate limit serves +both. A flow's treatment then depends on how its datagrams interleave with the others'. + +## Batches + +A paced hop, a Wi-Fi access point or a cellular scheduler bunches traffic: nothing moves for a while, then +several datagrams arrive at once. A batch holds datagrams until `count` are waiting, or until `window` +after the first arrived, then sends them all when the latest would have left. No delay or jitter produces +that clump, and it is what a receiver's jitter estimate has to cope with. + +## Steps + +A step changes a direction's delay, jitter, loss, reorder or rate limit once the run reaches `at`, counted +from when forwarding started. It changes only what it names, so a later step puts one knob back. A rate +step keeps the old bucket's credit, or its debt, clipped to the new one, and a limit that was not there +starts full. A step can add or change a rate limit, never remove one. Steps come from a profile file. + +## Profiles + +A profile file names both directions' treatment, the seed, and whether the path is shared. `up` is from +the client to the target, `down` the way back. Every field of a direction is optional and off when left +out, and unknown fields are refused. + +```toml +name = "example" +seed = 7 +shared = true + +[up] +delay = "5ms" +jitter = "5ms" +jitter_model = "gaussian" # required once the delay varies, through jitter or steps +loss = 0.02 +reorder = 0.01 # skips the delay, so it needs one + +[up.rate] +bits_per_second = 2000000 +burst = 12500 +queue = "250ms" + +[up.batch] +count = 7 +window = "160ms" + +[[up.steps]] # at 30s the delay grows, +at = "30s" +delay = "60ms" + +[[up.steps]] # and at 60s it recovers +at = "60s" +delay = "5ms" + +[down] +delay = "5ms" +``` + +`--profile` takes a file's path, or one of the built-ins in `profiles/`, embedded in the binary. The +audio-quality lane (`test/audio-quality/`) runs the first five. + +| Name | What it models | +| --- | --- | +| `near-zero` | The control. The shaper is in the path but treats nothing. | +| `mild` | A healthy wired LAN: 5ms delay, 5ms sigma, in order. | +| `bursty` | A paced hop: seven datagrams per 160ms window, released together. | +| `step` | 5ms for thirty seconds, then 60ms, with no recovery. | +| `high-rtt` | An intercontinental path: 75ms one way, 30ms sigma, in order. | +| `lossy` | 2% loss, with 1% of datagrams skipping a 20ms delay to overtake. | + +Each built-in is a shared path, as a browser page's sessions on one host would be. + +## TCP + +A relay serves `/certificate.sha256` over HTTP on the port number it serves QUIC on, and a browser fetches +it before it dials WebTransport. With `tcp_passthrough`, the shaper listens for TCP on its own port and +pipes each connection to the target untouched, so the page loads through it. TCP is never impaired: a +reliable transport cannot shed load, so shaping it would only measure how TCP retransmits. + +## Counters + +`Shaper::stats` counts what each direction did, summed over every client. + +| Counter | Meaning | +| --- | --- | +| `packets` | Datagrams received. | +| `lost` | Datagrams dropped by `loss`. | +| `overflowed` | Datagrams dropped because the rate limit's queue was full. | +| `throttled` | Datagrams that waited for the rate limit. | +| `delayed` | Datagrams given a nonzero delay, by the profile or by a batch holding them. | +| `reordered` | Datagrams sent ahead of the delay, overtaking any still in flight. | + +## Report + +`--report ` writes `{"profile", "seed", "up", "down"}` at exit, before the verdict, so a run that +fails it still leaves its counters behind. `profile` is the profile's name, or null when the flags built +it, and `up` and `down` hold the counters above. `--report-interval` prints the same object as one line on +stdout each period, which also survives a shaper killed before it could write the file. + +A profile that silently did nothing turns an impaired run into an unimpaired pass. `verify` catches that +for the shaper's own exit; a harness grading a run should also check the report, where `near-zero` is the +only profile for which a `delayed` of zero is the right answer. Keep the name, the seed and the counters +with the run's artifacts: the seed is what turns a failing run into one that can be looked at again. diff --git a/rs/moq-shaper/src/lib.rs b/rs/moq-shaper/src/lib.rs index 7d0f17d99a..fa84248661 100644 --- a/rs/moq-shaper/src/lib.rs +++ b/rs/moq-shaper/src/lib.rs @@ -13,6 +13,10 @@ //! A profile that silently did nothing would turn an impaired run into an //! unimpaired pass, so [`Shaper::verify`] fails when an impairment the profile //! configures never acted and the traffic makes that silence implausible. +//! +//! A [`Setup`] adds opt-in options to a [`Config`]: a TCP passthrough, a jitter +//! model that keeps the order, one link shared by every client, batches, steps, +//! and named profiles loaded as a [`Preset`]. The README says why each exists. use std::{ cmp::Reverse, From 53137d666a46dcea107d0bbd5c719af7fbe916f9 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 22:49:39 -0700 Subject: [PATCH 11/19] test(audio-quality): grade bursty and changing paths Co-Authored-By: GPT-6 --- .github/workflows/nightly.yml | 4 +- Cargo.toml | 1 + quest/m0/audio-quality-harness/README.md | 14 +- .../audio-quality-harness/shaper-profiles.md | 34 --- quest/m1/audio-quality-native.md | 4 - rs/moq-shaper/Cargo.toml | 2 +- rs/moq-shaper/README.md | 12 +- rs/moq-shaper/profiles/high-rtt.toml | 15 -- rs/moq-shaper/profiles/lossy.toml | 16 -- rs/moq-shaper/src/lib.rs | 119 ++-------- rs/moq-shaper/src/main.rs | 10 +- rs/moq-shaper/src/preset.rs | 3 - rs/moq-shaper/tests/cli.rs | 40 ++-- test/audio-quality/README.md | 21 +- test/audio-quality/budgets.json | 212 +++++++++++++----- test/audio-quality/clients/js/src/run.test.ts | 30 +++ test/audio-quality/run.sh | 15 +- test/justfile | 2 +- 18 files changed, 267 insertions(+), 287 deletions(-) delete mode 100644 quest/m0/audio-quality-harness/shaper-profiles.md delete mode 100644 rs/moq-shaper/profiles/high-rtt.toml delete mode 100644 rs/moq-shaper/profiles/lossy.toml create mode 100644 test/audio-quality/clients/js/src/run.test.ts diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index eed4b612bc..a34677533c 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -18,7 +18,7 @@ name: Nightly # the wheel builds in a manylinux container, the Kotlin/Swift/Go/Dart packages # link the FFI on every platform) and nothing notices until the tag is pushed. # -# `audio-quality` is here because of what it measures. It plays sixteen rows of +# `audio-quality` is here because of what it measures. It plays twenty-four rows of # audio in real time, a minute each, which no merge gate should carry, and every # number it grades is a timing one that moves with whatever else the runner is # doing. A per-PR arm would fail on unrelated load often enough to be ignored; @@ -189,7 +189,7 @@ jobs: audio-quality: name: Audio quality runs-on: ubuntu-24.04-arm - # Sixteen rows at a minute each, plus the relay, CLI, and shaper built from source. + # Twenty-four rows at a minute each, plus the relay, CLI, and shaper built from source. timeout-minutes: 60 steps: - name: Free disk space diff --git a/Cargo.toml b/Cargo.toml index 789ecbbfd0..8665f822e1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -124,6 +124,7 @@ hex = "0.4" # HMAC-SHA256 for the mDNS membership proofs (moq-tokio's `mdns` feature). hmac = "0.13" humantime = "2.3" +humantime-serde = "1.1.1" io-uring = "0.7.14" jsonwebtoken = "11" kio = { version = "0.6.1", path = "rs/kio" } diff --git a/quest/m0/audio-quality-harness/README.md b/quest/m0/audio-quality-harness/README.md index e3779f51ba..35388e5b76 100644 --- a/quest/m0/audio-quality-harness/README.md +++ b/quest/m0/audio-quality-harness/README.md @@ -23,13 +23,12 @@ are. The native lane moved to m1 as a standalone quest (decided in the same budgets and metric schema so the two implementations can be compared rather than merely both passing. -The browser lane has landed as `test/audio-quality/`, upstreamed from the -reporter's fork on #3477 (`fperex/moq`, branch `debug-findings-solution`) on -its own, without the fork's player, estimator, or shaper changes. That leaves -the two things that needed them: real arrival traces, which the [jitter -target's watch quest](/quest/m0/audio-jitter-target/watch.md) replays, and the -bursty and step profiles, which need shaper features the fork has and `main` -does not. +The browser lane and its opt-in shaper profiles have landed in `test/audio-quality/`, +upstreamed from the reporter's fork on #3477 (`fperex/moq`, branch +`debug-findings-solution`), without the fork's player or estimator changes. +The matrix includes bursty and mid-run step paths, and mild/wide jitter preserves +packet order. Real arrival traces remain for the [jitter target's watch +quest](/quest/m0/audio-jitter-target/watch.md) to replay. Jitter comes from the seeded userspace UDP shaper the transport drills run under (`rs/moq-shaper`, documented in `test/drill/README.md`), not from a fake @@ -49,7 +48,6 @@ each of those dimensions moves the expected floor. ## Required - [Traces](/quest/m0/audio-quality-harness/traces.md) - real arrival traces recorded, checked in, and graded as replay rows -- [Shaper profiles](/quest/m0/audio-quality-harness/shaper-profiles.md) - the bursty and mid-run step profiles, once moq-shaper can express them ## Related diff --git a/quest/m0/audio-quality-harness/shaper-profiles.md b/quest/m0/audio-quality-harness/shaper-profiles.md deleted file mode 100644 index 71536c518d..0000000000 --- a/quest/m0/audio-quality-harness/shaper-profiles.md +++ /dev/null @@ -1,34 +0,0 @@ -# [M] The audio quality matrix grades a bursty path and a mid-run step - -## Goal - -`just test audio-quality` gains the two profiles `moq-shaper` cannot express -today: `bursty`, which releases datagrams in batches (the flush-span shape from -#3477, seven datagrams per 160 ms window), and `step`, which changes the -profile part-way through a row and forces the adaptive target to move. Both get -budgets in `test/audio-quality/budgets.json` and run nightly with the rest. - -## Plan - -The browser lane landed on the shaper as it is: one uniform profile both ways, -fixed for the life of the process. Its `mild` and `wide` profiles spread -arrivals with uniform jitter, which also reorders datagrams whenever the -jitter exceeds their spacing, so `wide` measures reordering as much as spread. - -The reporter's fork (`fperex/moq`, branch `debug-findings-solution`, -`rs/moq-shaper`) already has what is missing, as separate commits: batches that -release datagrams together, steps that change a profile mid-run, an -order-preserving gaussian jitter model, named profiles loaded from TOML, and a -JSON report of the counters. Upstream the batch, step, and order-preserving -jitter changes with the fork author's credit, keeping each behind the options -the drills do not set so their lane is unchanged. Skip TCP passthrough: the -harness answers the page's certificate fetch itself. - -Then add `bursty` and `step` to `run.sh`, refuse a `--duration` that ends -before the step, and switch `mild` and `wide` to the order-preserving jitter so -their names mean spread alone. Re-record the budgets for every row whose profile -changed. - -## Related - -- [Audio jitter target](/quest/m0/audio-jitter-target/README.md) - the estimator the step profile exercises diff --git a/quest/m1/audio-quality-native.md b/quest/m1/audio-quality-native.md index 11373ae103..232cc61deb 100644 --- a/quest/m1/audio-quality-native.md +++ b/quest/m1/audio-quality-native.md @@ -45,7 +45,3 @@ Standalone in m1 rather than a child of the m0 [Audio quality harness](/quest/m0/audio-quality-harness/README.md) line (decided in the 2026-09-28 quest audit): nothing in m0 waits on it. The native jitter target it grades is done on the jitter target line. - -## Related - -- [Shaper profiles](/quest/m0/audio-quality-harness/shaper-profiles.md) - the bursty and step profiles both lanes should run diff --git a/rs/moq-shaper/Cargo.toml b/rs/moq-shaper/Cargo.toml index a18c3cc8fc..ebeb0190d7 100644 --- a/rs/moq-shaper/Cargo.toml +++ b/rs/moq-shaper/Cargo.toml @@ -24,7 +24,7 @@ humantime-serde = { workspace = true } rand = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } -tokio = { workspace = true, features = ["io-util", "macros", "net", "rt-multi-thread", "signal", "sync", "time"] } +tokio = { workspace = true, features = ["macros", "net", "rt-multi-thread", "signal", "sync", "time"] } toml = { workspace = true } [dev-dependencies] diff --git a/rs/moq-shaper/README.md b/rs/moq-shaper/README.md index 8eb817054a..89ad49d53b 100644 --- a/rs/moq-shaper/README.md +++ b/rs/moq-shaper/README.md @@ -38,7 +38,6 @@ profile never acted. | `--batch`, `--batch-window` | Hold datagrams until this many wait, or the window closes, then release them together. See [Batches](#batches). | | `--shared` | Every client shares one link each way. See [Shared path](#shared-path). | | `--profile` | A built-in profile's name, or a profile TOML file, in place of the flags that shape the path. See [Profiles](#profiles). | -| `--tcp-passthrough` | Also pipe TCP on the listening port to the target, untouched. See [TCP](#tcp). | | `--report` | Write the profile, seed and counters to this file as JSON at exit. See [Report](#report). | | `--report-interval` | Print the same JSON as one line on stdout this often. | @@ -133,7 +132,7 @@ delay = "5ms" ``` `--profile` takes a file's path, or one of the built-ins in `profiles/`, embedded in the binary. The -audio-quality lane (`test/audio-quality/`) runs the first five. +audio-quality lane (`test/audio-quality/`) uses `bursty` and `step`, plus its own flag profiles. | Name | What it models | | --- | --- | @@ -141,18 +140,9 @@ audio-quality lane (`test/audio-quality/`) runs the first five. | `mild` | A healthy wired LAN: 5ms delay, 5ms sigma, in order. | | `bursty` | A paced hop: seven datagrams per 160ms window, released together. | | `step` | 5ms for thirty seconds, then 60ms, with no recovery. | -| `high-rtt` | An intercontinental path: 75ms one way, 30ms sigma, in order. | -| `lossy` | 2% loss, with 1% of datagrams skipping a 20ms delay to overtake. | Each built-in is a shared path, as a browser page's sessions on one host would be. -## TCP - -A relay serves `/certificate.sha256` over HTTP on the port number it serves QUIC on, and a browser fetches -it before it dials WebTransport. With `tcp_passthrough`, the shaper listens for TCP on its own port and -pipes each connection to the target untouched, so the page loads through it. TCP is never impaired: a -reliable transport cannot shed load, so shaping it would only measure how TCP retransmits. - ## Counters `Shaper::stats` counts what each direction did, summed over every client. diff --git a/rs/moq-shaper/profiles/high-rtt.toml b/rs/moq-shaper/profiles/high-rtt.toml deleted file mode 100644 index 1f600fbbf3..0000000000 --- a/rs/moq-shaper/profiles/high-rtt.toml +++ /dev/null @@ -1,15 +0,0 @@ -# An intercontinental path: 75ms each way with a wide spread on top, queued in -# order. -name = "high-rtt" -seed = 5 -shared = true - -[up] -delay = "75ms" -jitter = "30ms" -jitter_model = "gaussian" - -[down] -delay = "75ms" -jitter = "30ms" -jitter_model = "gaussian" diff --git a/rs/moq-shaper/profiles/lossy.toml b/rs/moq-shaper/profiles/lossy.toml deleted file mode 100644 index 816063a752..0000000000 --- a/rs/moq-shaper/profiles/lossy.toml +++ /dev/null @@ -1,16 +0,0 @@ -# A path that loses 2%, and on which 1% of what survives skips the 20ms delay -# the rest take, arriving ahead of what is still in flight: the loss and the -# reordering a recovery path has to cope with. -name = "lossy" -seed = 6 -shared = true - -[up] -delay = "20ms" -loss = 0.02 -reorder = 0.01 - -[down] -delay = "20ms" -loss = 0.02 -reorder = 0.01 diff --git a/rs/moq-shaper/src/lib.rs b/rs/moq-shaper/src/lib.rs index fa84248661..5b0e0f24ca 100644 --- a/rs/moq-shaper/src/lib.rs +++ b/rs/moq-shaper/src/lib.rs @@ -14,7 +14,7 @@ //! unimpaired pass, so [`Shaper::verify`] fails when an impairment the profile //! configures never acted and the traffic makes that silence implausible. //! -//! A [`Setup`] adds opt-in options to a [`Config`]: a TCP passthrough, a jitter +//! A [`Setup`] adds opt-in options to a [`Config`]: a jitter //! model that keeps the order, one link shared by every client, batches, steps, //! and named profiles loaded as a [`Preset`]. The README says why each exists. @@ -37,12 +37,7 @@ pub use preset::Preset; use anyhow::Context; use rand::{RngExt, SeedableRng, rngs::Xoshiro256PlusPlus}; use serde::{Deserialize, Serialize}; -use tokio::{ - net::{TcpListener, TcpStream, UdpSocket}, - sync::mpsc, - task::JoinSet, - time::Instant, -}; +use tokio::{net::UdpSocket, sync::mpsc, task::JoinSet, time::Instant}; /// How one direction of the path treats each datagram. /// @@ -315,13 +310,6 @@ pub struct Config { pub struct Setup { /// Where to listen and forward, the seed, and each direction's profile. pub config: Config, - /// Also accept TCP on the listening port and pipe it to the target untouched. - /// - /// A relay serves HTTP on the port number it serves QUIC on, and a browser - /// fetches the certificate hash from it before it dials WebTransport. TCP is - /// never impaired: a reliable transport cannot shed load, so shaping it - /// would measure how TCP retransmits. - pub tcp_passthrough: bool, /// Every client shares one link each way, the way clients behind one access /// link do, rather than each getting its own. /// @@ -339,7 +327,6 @@ impl From for Setup { fn from(config: Config) -> Self { Self { config, - tcp_passthrough: false, shared: false, up: Options::default(), down: Options::default(), @@ -412,16 +399,6 @@ impl Shaper { .with_context(|| format!("bind {}", config.bind))?; let addr = listen.local_addr()?; - // The port the UDP socket got, since a relay serves HTTP on its QUIC port. - let tcp = match setup.tcp_passthrough { - true => Some( - TcpListener::bind(addr) - .await - .with_context(|| format!("bind the TCP passthrough on {addr}"))?, - ), - false => None, - }; - let tally = Arc::new([Tally::default(), Tally::default()]); let failed = Arc::new(OnceLock::new()); let task = tokio::spawn({ @@ -429,7 +406,7 @@ impl Shaper { let tally = tally.clone(); let failed = failed.clone(); async move { - if let Err(err) = run(Arc::new(listen), tcp, setup, tally).await { + if let Err(err) = run(Arc::new(listen), setup, tally).await { let _ = failed.set(format!("{err:#}")); } } @@ -527,12 +504,7 @@ fn bump(counter: &AtomicU64) { /// A flow is a socket of its own toward the target, so the target sees one /// address per client just as it would without the shaper in the way. Each /// flow takes a link of its own each way, unless the path is shared. -async fn run( - listen: Arc, - tcp: Option, - setup: Setup, - tally: Arc<[Tally; 2]>, -) -> anyhow::Result<()> { +async fn run(listen: Arc, setup: Setup, tally: Arc<[Tally; 2]>) -> anyhow::Result<()> { let config = &setup.config; let mut flows = HashMap::::new(); let mut tasks = JoinSet::new(); @@ -553,11 +525,7 @@ async fn run( res.context("flow task panicked")??; continue; } - res = accept(tcp.as_ref()) => { - let (stream, _) = res.context("accept a TCP connection")?; - tasks.spawn(pipe(stream, config.target)); - continue; - } + }; let now = Instant::now(); @@ -622,23 +590,6 @@ fn link( ))) } -/// The next TCP connection, or never without a passthrough. -async fn accept(tcp: Option<&TcpListener>) -> std::io::Result<(TcpStream, SocketAddr)> { - match tcp { - Some(listener) => listener.accept().await, - None => std::future::pending().await, - } -} - -/// Copy one TCP connection to the target and back, untouched. -async fn pipe(mut client: TcpStream, target: SocketAddr) -> anyhow::Result<()> { - // Either end refusing or hanging up ends that connection, not the shaper. - if let Ok(mut server) = TcpStream::connect(target).await { - let _ = tokio::io::copy_bidirectional(&mut client, &mut server).await; - } - Ok(()) -} - /// A socket that can reach `target`: loopback for a loopback target, so the /// shaper never opens a port beyond the host when it does not need to. async fn bind_toward(target: SocketAddr) -> anyhow::Result { @@ -831,7 +782,9 @@ impl Link { while self.steps.last().is_some_and(|step| self.start + step.at <= now) { let step = self.steps.pop().expect("a step is due"); if let Some(rate) = &step.rate { - self.full_at = self.refilled(rate, now); + // Credit accrues at the new rate from the scheduled transition, + // even when no datagram arrived at that instant. + self.full_at = self.refilled(rate, self.start + step.at); } step.apply(&mut self.profile); } @@ -1184,56 +1137,6 @@ mod tests { refused(undelayed, "needs a delay").await; } - #[tokio::test] - async fn tcp_passes_through_untouched() { - use tokio::io::{AsyncReadExt, AsyncWriteExt}; - - // A relay answering HTTP on its QUIC port, reduced to an echo. - let server = TcpListener::bind(LOCALHOST).await.unwrap(); - let target = server.local_addr().unwrap(); - tokio::spawn(async move { - let (mut stream, _) = server.accept().await.unwrap(); - let mut buf = [0u8; 64]; - let size = stream.read(&mut buf).await.unwrap(); - stream.write_all(&buf[..size]).await.unwrap(); - }); - - // A profile that would lose every datagram, to show TCP skips it. - let blackhole = Profile { - loss: 1.0, - ..Default::default() - }; - let config = Config { - bind: LOCALHOST, - target, - seed: 13, - up: blackhole.clone(), - down: blackhole, - }; - let shaper = Shaper::bind(Setup { - tcp_passthrough: true, - ..config.into() - }) - .await - .unwrap(); - - let mut client = TcpStream::connect(shaper.addr()).await.unwrap(); - client.write_all(b"/certificate.sha256").await.unwrap(); - let mut buf = [0u8; 64]; - let size = tokio::time::timeout(Duration::from_secs(2), client.read(&mut buf)) - .await - .unwrap() - .unwrap(); - assert_eq!(&buf[..size], b"/certificate.sha256"); - assert_eq!(shaper.stats(), Stats::default(), "TCP reached the datagram path"); - } - - #[tokio::test] - async fn tcp_is_refused_without_the_passthrough() { - let (shaper, _client) = setup(1, Profile::default(), Profile::default()).await; - assert!(TcpStream::connect(shaper.addr()).await.is_err()); - } - /// An echo server, a shaper `setup` builds from a config aimed at it, and a /// client dialing the shaper. async fn shaped(setup: impl FnOnce(Config) -> Setup) -> (Shaper, UdpSocket) { @@ -1841,6 +1744,12 @@ mod tests { // 100 bytes of the backlog are still owed, and now drain in 50ms, so // a fresh 100 bytes leave once those and themselves are paid. assert_eq!(owed(&mut link, start + ms(100), &[100]), [ms(100)]); + + // A quiet link still changes rates at the step, not at its next arrival. + let (queue, _) = mpsc::unbounded_channel(); + let mut link = Link::new(&profile, &options, 7, 0, start, queue); + owed(&mut link, start, &[100, 100, 100]); + assert_eq!(owed(&mut link, start + ms(200), &[100]), [ms(0)]); } #[tokio::test] diff --git a/rs/moq-shaper/src/main.rs b/rs/moq-shaper/src/main.rs index 0fac2dacd7..c16eb30578 100644 --- a/rs/moq-shaper/src/main.rs +++ b/rs/moq-shaper/src/main.rs @@ -65,9 +65,6 @@ struct Args { /// Every client shares one link each way, instead of each getting its own. #[arg(long)] shared: bool, - /// Also pipe TCP on the listening port to the target, untouched. - #[arg(long)] - tcp_passthrough: bool, /// Write the profile, seed and counters to this file as JSON at exit. #[arg(long)] report: Option, @@ -87,8 +84,12 @@ enum JitterModel { #[tokio::main] async fn main() -> anyhow::Result<()> { let args = Args::parse(); + anyhow::ensure!( + args.report_interval.is_none_or(|period| !period.is_zero()), + "the report interval must be positive" + ); - let (name, mut setup) = match &args.profile { + let (name, setup) = match &args.profile { Some(profile) => { let preset = moq_shaper::Preset::load(profile)?; let mut setup = preset.setup(args.listen, args.target); @@ -99,7 +100,6 @@ async fn main() -> anyhow::Result<()> { } None => (None, flags(&args)), }; - setup.tcp_passthrough = args.tcp_passthrough; let treatment = match &name { Some(name) => format!("profile {name}"), diff --git a/rs/moq-shaper/src/preset.rs b/rs/moq-shaper/src/preset.rs index 56c01a1778..e8cd8095a9 100644 --- a/rs/moq-shaper/src/preset.rs +++ b/rs/moq-shaper/src/preset.rs @@ -11,8 +11,6 @@ const BUILTIN: &[(&str, &str)] = &[ ("mild", include_str!("../profiles/mild.toml")), ("bursty", include_str!("../profiles/bursty.toml")), ("step", include_str!("../profiles/step.toml")), - ("high-rtt", include_str!("../profiles/high-rtt.toml")), - ("lossy", include_str!("../profiles/lossy.toml")), ]; /// A named, seeded treatment of both directions, loaded from a TOML profile. @@ -60,7 +58,6 @@ impl Preset { up: self.up.0.clone(), down: self.down.0.clone(), }, - tcp_passthrough: false, shared: self.shared, up: self.up.1.clone(), down: self.down.1.clone(), diff --git a/rs/moq-shaper/tests/cli.rs b/rs/moq-shaper/tests/cli.rs index fb37ba9d1f..8c15351647 100644 --- a/rs/moq-shaper/tests/cli.rs +++ b/rs/moq-shaper/tests/cli.rs @@ -1,31 +1,23 @@ -//! The binary end to end: a profile by name, the TCP passthrough, and the +//! The binary end to end: a profile by name and the //! counters it reports while it runs and when it stops. #![cfg(unix)] use std::{ - io::{BufRead, BufReader, Read, Write}, - net::{SocketAddr, TcpListener, TcpStream, UdpSocket}, + io::{BufRead, BufReader}, + net::{SocketAddr, UdpSocket}, process::{Command, Stdio}, time::Duration, }; #[test] fn a_profile_run_reports_its_counters() { - // The target answers TCP on the port number it takes datagrams on, as a relay does. let target = UdpSocket::bind("127.0.0.1:0").unwrap(); let target_addr = target.local_addr().unwrap(); - let http = TcpListener::bind(target_addr).unwrap(); - std::thread::spawn(move || { - let (mut stream, _) = http.accept().unwrap(); - let mut buf = [0u8; 64]; - let size = stream.read(&mut buf).unwrap(); - stream.write_all(&buf[..size]).unwrap(); - }); let report = std::env::temp_dir().join(format!("moq-shaper-report-{}.json", std::process::id())); let mut shaper = Command::new(env!("CARGO_BIN_EXE_moq-shaper")) .args(["--listen", "127.0.0.1:0", "--target", &target_addr.to_string()]) - .args(["--profile", "mild", "--seed", "7", "--tcp-passthrough"]) + .args(["--profile", "mild", "--seed", "7"]) .args(["--report-interval", "100ms", "--report", report.to_str().unwrap()]) .stdout(Stdio::piped()) .spawn() @@ -48,12 +40,6 @@ fn a_profile_run_reports_its_counters() { target.recv_from(&mut buf).expect("a datagram never arrived"); } - let mut tcp = TcpStream::connect(addr).unwrap(); - tcp.write_all(b"/certificate.sha256").unwrap(); - let mut buf = [0u8; 64]; - let size = tcp.read(&mut buf).unwrap(); - assert_eq!(&buf[..size], b"/certificate.sha256"); - // A line each interval, once the traffic has been counted. let interim = loop { line.clear(); @@ -81,3 +67,21 @@ fn a_profile_run_reports_its_counters() { assert_eq!(json["up"]["delayed"], 50); assert_eq!(json["up"]["lost"], 0); } + +#[test] +fn a_zero_report_interval_is_refused_before_binding() { + let result = Command::new(env!("CARGO_BIN_EXE_moq-shaper")) + .args([ + "--listen", + "127.0.0.1:0", + "--target", + "127.0.0.1:9", + "--report-interval", + "0s", + ]) + .output() + .unwrap(); + assert!(!result.status.success()); + assert!(result.stdout.is_empty(), "bound before refusing the interval"); + assert!(String::from_utf8_lossy(&result.stderr).contains("report interval must be positive")); +} diff --git a/test/audio-quality/README.md b/test/audio-quality/README.md index 242660731f..c9640eb993 100644 --- a/test/audio-quality/README.md +++ b/test/audio-quality/README.md @@ -14,7 +14,7 @@ just test audio-quality --out ~/runs/after # keep the run di just test audio-quality --enforce # fail on the budgets ``` -The matrix is codec x profile x ring: 16 rows, about 20 minutes. `--profiles`, `--rings`, and +The matrix is codec x profile x ring: 24 rows, about 30 minutes. `--profiles`, `--rings`, and `--codecs` take comma-separated lists; `--seed` replays a given impairment. The table prints either way, and only `--enforce` (which the nightly job passes) turns a breach into a failed run. @@ -33,17 +33,18 @@ decides which ring ran. | Profile | Path | Delay | | --- | --- | --- | | `near-zero` | through the shaper, untreated | auto | -| `mild` | 5 ms one way, plus or minus 5 ms uniform | auto | -| `wide` | 40 ms one way, plus or minus 40 ms uniform | auto | +| `mild` | 5 ms one way, 5 ms Gaussian sigma, in order | auto | +| `wide` | 40 ms one way, 40 ms Gaussian sigma, in order | auto | +| `bursty` | batches of seven datagrams, released within 160 ms | auto | +| `step` | 5 ms one way, stepping to 60 ms at 30 s | auto | | `fixed-250` | through the shaper, untreated | a fixed 250 ms | `fixed-250` is the control: every other row adapts, so without it the whole matrix could pass while a fixed preset regressed, and a fixed preset is what many viewers land on. Loss, reorder, and rate limits stay off: the buffer's job is absorbing arrival spread, and congestion response would make a -failure hard to attribute. The shaper's uniform jitter does reorder datagrams whose spacing is -smaller than it, so `wide` measures some reordering along with the spread. The bursty and mid-run -step profiles need shaper features that are not here yet -([quest](../../quest/m0/audio-quality-harness/shaper-profiles.md)). +failure hard to attribute. The `mild` and `wide` profiles preserve datagram order and share one path across the page's sessions. +`bursty` and `step` use the shaper's named profiles on that same shared path. The step is timed from +shaper startup, before the page connects; `--duration` must exceed 30 s so playback spans it. The AAC arm keeps ffmpeg's default PES packing, whose multi-frame bursts are the flush-span shape the reporter measured on the public relay (#3477). The source is a sine tone rather than a film, so a @@ -172,9 +173,9 @@ clients/js/ The harness is the reporter's, from #3477: fperex built it on their fork `fperex/moq` (branches `debug/rt-audio` and `debug-findings-solution`), with the black-box probe, the analyzer's skip-ahead rule, the Playwright matrix over `moq-shaper`, the budget file graded under `--enforce`, and the -nightly job. This lane upstreams it on its own, without the fork's player, estimator, or shaper -changes: the underrun count comes from a tap on the output rather than a counter in the player, and -the profiles are the ones the shaper on `main` can express. +nightly job. The harness and opt-in shaper features preserve that author's credit. The player and estimator +changes remain separate: the underrun count comes from a tap on the output rather than a counter in the player, and +the profiles now include the fork's batch, step, and order-preserving jitter treatments. ## Not covered diff --git a/test/audio-quality/budgets.json b/test/audio-quality/budgets.json index 0cdde6b25c..16971ef4d0 100644 --- a/test/audio-quality/budgets.json +++ b/test/audio-quality/budgets.json @@ -1,6 +1,34 @@ { - "note": "Measured 2026-09-28 on a shared Linux workstation: two full 60 s runs and one 20 s run, seed 7. Each ceiling is twice the worse run, rounded up to two significant figures, with a floor (5 underruns/min, 20 gap ms/min, 1 skip/min, 100 discarded ms/min, 1% stalled, 1% silence) so one runner hiccup does not fail a clean row; target_ms_p95 is the worst measured plus one 20 ms bucket. The fixed-250 control measured zero on every counter. Every auto row records today's shallow target (#3477: 43 ms for Opus, underrunning thousands of quanta a minute), so its ceilings catch a regression past that floor and nothing better; the jitter target work is expected to tighten them. Not yet measured on the nightly runner, which has no audio device; re-record from its first runs.", + "note": "Measured 2026-09-28 on a shared Linux workstation: two full 60 s runs and one 20 s run, seed 7. Each ceiling is twice the worse run, rounded up to two significant figures, with a floor (5 underruns/min, 20 gap ms/min, 1 skip/min, 100 discarded ms/min, 1% stalled, 1% silence) so one runner hiccup does not fail a clean row; target_ms_p95 is the worst measured plus one 20 ms bucket. The fixed-250 control measured zero on every counter. Every auto row records today's shallow target (#3477: 43 ms for Opus, underrunning thousands of quanta a minute), so its ceilings catch a regression past that floor and nothing better; the jitter target work is expected to tighten them. Not yet measured on the nightly runner, which has no audio device; re-record from its first runs. Updated 2026-09-29: mild and wide now use shared order-preserving Gaussian jitter; bursty and step are new. Those 16 rows were re-recorded for 60 s each, seed 7, on the same shared Linux workstation with concurrent bounded builds. Their ceilings use twice the measurement rounded upward to two significant figures and the same floors above; target_ms_p95 adds one 20 ms bucket. Other rows retain their previous measurements. These remain current-player regression ceilings, not target audio quality; re-record on the nightly runner before tightening them.", "rows": [ + { + "runtime": "chromium", + "codec": "aac", + "rate": 44100, + "profile": "bursty", + "ring": "isolated", + "underruns_per_min": 1100.0, + "underrun_ms_per_min": 3000.0, + "skip_aheads_per_min": 6.6, + "discarded_ms_per_min": 500.0, + "stalled_share": 0.01, + "silence_share": 0.05, + "target_ms_p95": 404.8 + }, + { + "runtime": "chromium", + "codec": "aac", + "rate": 44100, + "profile": "bursty", + "ring": "plain", + "underruns_per_min": 740.0, + "underrun_ms_per_min": 2200.0, + "skip_aheads_per_min": 4.4, + "discarded_ms_per_min": 350.0, + "stalled_share": 0.01, + "silence_share": 0.034, + "target_ms_p95": 368.5 + }, { "runtime": "chromium", "codec": "aac", @@ -35,13 +63,13 @@ "rate": 44100, "profile": "mild", "ring": "isolated", - "underruns_per_min": 270, - "underrun_ms_per_min": 780, - "skip_aheads_per_min": 2.2, - "discarded_ms_per_min": 160, + "underruns_per_min": 320.0, + "underrun_ms_per_min": 890.0, + "skip_aheads_per_min": 1.0, + "discarded_ms_per_min": 100.0, "stalled_share": 0.01, - "silence_share": 0.012, - "target_ms_p95": 210 + "silence_share": 0.014, + "target_ms_p95": 211 }, { "runtime": "chromium", @@ -49,13 +77,13 @@ "rate": 44100, "profile": "mild", "ring": "plain", - "underruns_per_min": 570, - "underrun_ms_per_min": 1700, - "skip_aheads_per_min": 2.2, - "discarded_ms_per_min": 120, + "underruns_per_min": 340.0, + "underrun_ms_per_min": 970.0, + "skip_aheads_per_min": 1.0, + "discarded_ms_per_min": 100.0, "stalled_share": 0.01, - "silence_share": 0.026, - "target_ms_p95": 210 + "silence_share": 0.014, + "target_ms_p95": 206 }, { "runtime": "chromium", @@ -89,15 +117,43 @@ "runtime": "chromium", "codec": "aac", "rate": 44100, - "profile": "wide", + "profile": "step", "ring": "isolated", - "underruns_per_min": 430, - "underrun_ms_per_min": 1200, + "underruns_per_min": 970.0, + "underrun_ms_per_min": 2800.0, "skip_aheads_per_min": 6.6, - "discarded_ms_per_min": 330, + "discarded_ms_per_min": 380.0, "stalled_share": 0.01, - "silence_share": 0.018, - "target_ms_p95": 250 + "silence_share": 0.042, + "target_ms_p95": 207.3 + }, + { + "runtime": "chromium", + "codec": "aac", + "rate": 44100, + "profile": "step", + "ring": "plain", + "underruns_per_min": 450.0, + "underrun_ms_per_min": 1200.0, + "skip_aheads_per_min": 2.2, + "discarded_ms_per_min": 120.0, + "stalled_share": 0.01, + "silence_share": 0.02, + "target_ms_p95": 212.3 + }, + { + "runtime": "chromium", + "codec": "aac", + "rate": 44100, + "profile": "wide", + "ring": "isolated", + "underruns_per_min": 140.0, + "underrun_ms_per_min": 360.0, + "skip_aheads_per_min": 2.2, + "discarded_ms_per_min": 140.0, + "stalled_share": 0.01, + "silence_share": 0.01, + "target_ms_p95": 319.8 }, { "runtime": "chromium", @@ -105,13 +161,41 @@ "rate": 44100, "profile": "wide", "ring": "plain", - "underruns_per_min": 540, - "underrun_ms_per_min": 1500, - "skip_aheads_per_min": 4.4, - "discarded_ms_per_min": 260, + "underruns_per_min": 330.0, + "underrun_ms_per_min": 940.0, + "skip_aheads_per_min": 1.0, + "discarded_ms_per_min": 100.0, "stalled_share": 0.01, - "silence_share": 0.03, - "target_ms_p95": 250 + "silence_share": 0.018, + "target_ms_p95": 308.5 + }, + { + "runtime": "chromium", + "codec": "opus", + "rate": 48000, + "profile": "bursty", + "ring": "isolated", + "underruns_per_min": 4800.0, + "underrun_ms_per_min": 13000.0, + "skip_aheads_per_min": 8.8, + "discarded_ms_per_min": 490.0, + "stalled_share": 0.018, + "silence_share": 0.2, + "target_ms_p95": 128 + }, + { + "runtime": "chromium", + "codec": "opus", + "rate": 48000, + "profile": "bursty", + "ring": "plain", + "underruns_per_min": 2900.0, + "underrun_ms_per_min": 7100.0, + "skip_aheads_per_min": 6.6, + "discarded_ms_per_min": 470.0, + "stalled_share": 0.028, + "silence_share": 0.13, + "target_ms_p95": 139.3 }, { "runtime": "chromium", @@ -147,12 +231,12 @@ "rate": 48000, "profile": "mild", "ring": "isolated", - "underruns_per_min": 4600, - "underrun_ms_per_min": 12000, - "skip_aheads_per_min": 1, - "discarded_ms_per_min": 100, + "underruns_per_min": 1300.0, + "underrun_ms_per_min": 2600.0, + "skip_aheads_per_min": 1.0, + "discarded_ms_per_min": 100.0, "stalled_share": 0.01, - "silence_share": 0.19, + "silence_share": 0.026, "target_ms_p95": 63 }, { @@ -161,12 +245,12 @@ "rate": 48000, "profile": "mild", "ring": "plain", - "underruns_per_min": 4800, - "underrun_ms_per_min": 12000, - "skip_aheads_per_min": 2.2, - "discarded_ms_per_min": 130, - "stalled_share": 0.028, - "silence_share": 0.22, + "underruns_per_min": 1400.0, + "underrun_ms_per_min": 2900.0, + "skip_aheads_per_min": 1.0, + "discarded_ms_per_min": 100.0, + "stalled_share": 0.01, + "silence_share": 0.032, "target_ms_p95": 63 }, { @@ -197,19 +281,47 @@ "silence_share": 0.19, "target_ms_p95": 63 }, + { + "runtime": "chromium", + "codec": "opus", + "rate": 48000, + "profile": "step", + "ring": "isolated", + "underruns_per_min": 1300.0, + "underrun_ms_per_min": 2700.0, + "skip_aheads_per_min": 1.0, + "discarded_ms_per_min": 100.0, + "stalled_share": 0.01, + "silence_share": 0.034, + "target_ms_p95": 63 + }, + { + "runtime": "chromium", + "codec": "opus", + "rate": 48000, + "profile": "step", + "ring": "plain", + "underruns_per_min": 2500.0, + "underrun_ms_per_min": 5700.0, + "skip_aheads_per_min": 2.2, + "discarded_ms_per_min": 100.0, + "stalled_share": 0.01, + "silence_share": 0.084, + "target_ms_p95": 63 + }, { "runtime": "chromium", "codec": "opus", "rate": 48000, "profile": "wide", "ring": "isolated", - "underruns_per_min": 8100, - "underrun_ms_per_min": 22000, - "skip_aheads_per_min": 29, - "discarded_ms_per_min": 1700, - "stalled_share": 0.56, - "silence_share": 0.73, - "target_ms_p95": 82 + "underruns_per_min": 2200.0, + "underrun_ms_per_min": 5600.0, + "skip_aheads_per_min": 2.2, + "discarded_ms_per_min": 100.0, + "stalled_share": 0.01, + "silence_share": 0.092, + "target_ms_p95": 150.5 }, { "runtime": "chromium", @@ -217,13 +329,13 @@ "rate": 48000, "profile": "wide", "ring": "plain", - "underruns_per_min": 6700, - "underrun_ms_per_min": 17000, - "skip_aheads_per_min": 18, - "discarded_ms_per_min": 1300, - "stalled_share": 0.49, - "silence_share": 0.75, - "target_ms_p95": 82 + "underruns_per_min": 2300.0, + "underrun_ms_per_min": 5700.0, + "skip_aheads_per_min": 4.4, + "discarded_ms_per_min": 200.0, + "stalled_share": 0.01, + "silence_share": 0.09, + "target_ms_p95": 154.3 } ] } diff --git a/test/audio-quality/clients/js/src/run.test.ts b/test/audio-quality/clients/js/src/run.test.ts new file mode 100644 index 0000000000..7cd6a67d10 --- /dev/null +++ b/test/audio-quality/clients/js/src/run.test.ts @@ -0,0 +1,30 @@ +import { expect, test } from "bun:test"; +import { fileURLToPath } from "node:url"; + +const script = fileURLToPath(new URL("../../../run.sh", import.meta.url)); + +test("a step row must play beyond the profile change", () => { + for (const duration of ["20", "30"]) { + const result = Bun.spawnSync(["bash", script, "--profiles", "step", "--duration", duration, "--list"]); + expect(result.exitCode).toBe(2); + expect(result.stderr.toString()).toContain("must exceed the step at 30s"); + } + const result = Bun.spawnSync(["bash", script, "--profiles", "step", "--duration", "31", "--list"]); + expect(result.exitCode).toBe(0); + expect(result.stdout.toString()).toContain("chromium-opus-48000-step-plain"); +}); + +test("short steady rows remain supported", () => { + const result = Bun.spawnSync(["bash", script, "--profiles", "mild", "--duration", "20", "--list"]); + expect(result.exitCode).toBe(0); +}); + +test("the nightly matrix includes both new profiles", () => { + const result = Bun.spawnSync(["bash", script, "--list"]); + expect(result.exitCode).toBe(0); + const rows = result.stdout.toString().trim().split("\n"); + expect(rows).toHaveLength(24); + for (const profile of ["bursty", "step"]) { + expect(rows.filter((row) => row.includes(`-${profile}-`))).toHaveLength(4); + } +}); diff --git a/test/audio-quality/run.sh b/test/audio-quality/run.sh index c10544597e..062576f313 100755 --- a/test/audio-quality/run.sh +++ b/test/audio-quality/run.sh @@ -15,7 +15,7 @@ source "$AQ_DIR/../lib/harness.sh" # Captured before the parse consumes it, so the rerun command carries every flag. RERUN="$(harness_env AQ_PROFILE RELAY_BIN)just test audio-quality$(harness_argv "$@")" -ALL_PROFILES=(near-zero mild wide fixed-250) +ALL_PROFILES=(near-zero mild wide bursty step fixed-250) ALL_RINGS=(isolated plain) ALL_CODECS=(opus aac) @@ -111,7 +111,13 @@ valid() { echo "error: unknown $name '$want' (known: $*)" >&2 exit 2 } -for p in "${PROFILES[@]}"; do valid "$p" profile "${ALL_PROFILES[@]}"; done +for p in "${PROFILES[@]}"; do + valid "$p" profile "${ALL_PROFILES[@]}" + if [[ "$p" == step ]] && ((DURATION <= 30)); then + echo "error: --duration must exceed the step at 30s" >&2 + exit 2 + fi +done for r in "${RINGS[@]}"; do valid "$r" ring "${ALL_RINGS[@]}"; done for c in "${CODECS[@]}"; do valid "$c" codec "${ALL_CODECS[@]}"; done @@ -141,8 +147,9 @@ delay_of() { shaper_of() { case "$1" in near-zero | fixed-250) ;; - mild) echo "--delay 5ms --jitter 5ms" ;; - wide) echo "--delay 40ms --jitter 40ms" ;; + mild) echo "--delay 5ms --jitter 5ms --jitter-model gaussian --shared" ;; + wide) echo "--delay 40ms --jitter 40ms --jitter-model gaussian --shared" ;; + bursty | step) echo "--profile $1" ;; esac } diff --git a/test/justfile b/test/justfile index d5ec917a5e..6461b9ad98 100644 --- a/test/justfile +++ b/test/justfile @@ -75,7 +75,7 @@ drill *args: # Plays a tone in headless Chromium through moq-shaper and grades what the listener # heard (underruns, skip-aheads, silence, the resolved delay) against -# audio-quality/budgets.json. The full matrix is 16 rows at 60 s; nightly runs it with +# audio-quality/budgets.json. The full matrix is 24 rows at 60 s; nightly runs it with # --enforce. Narrow it with `--profiles mild --codecs opus --rings plain --duration 20`. # See audio-quality/README.md. From 756200eb8585b654621f9ad563a7e7174a1adb0a Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 29 Sep 2026 10:12:12 -0700 Subject: [PATCH 12/19] fix(shaper): verify fails a step no traffic reached, and a late datagram starts a new batch `verify` now fails when a direction carried traffic but none of it reached a configured step, so a run that ended early no longer passes as stepped. A datagram that arrives after a batch's window closed, but before its timer fired, now releases the closed batch first and starts the next one. Co-Authored-By: Claude Opus 5.5 --- rs/moq-shaper/README.md | 6 ++-- rs/moq-shaper/src/lib.rs | 78 +++++++++++++++++++++++++++++++++------- 2 files changed, 70 insertions(+), 14 deletions(-) diff --git a/rs/moq-shaper/README.md b/rs/moq-shaper/README.md index 89ad49d53b..7adfee3d35 100644 --- a/rs/moq-shaper/README.md +++ b/rs/moq-shaper/README.md @@ -43,8 +43,9 @@ profile never acted. As a library, `Shaper::bind` takes a `Config`: where to listen and forward, the seed, and a `Profile` per direction. It also takes a `Setup`, which is a `Config` plus the opt-in options below, each off by -default. `Shaper::verify` fails when the shaper stopped forwarding, or when an impairment the profile -configures never acted and the traffic makes that silence implausible. The relay's drills +default. `Shaper::verify` fails when the shaper stopped forwarding, when an impairment the profile +configures never acted and the traffic makes that silence implausible, or when a direction carried +traffic but none of it reached a step. The relay's drills (`rs/moq-relay/tests/drills.rs`, described in `test/drill/README.md`) run every scenario through it. ## What a seed does and does not fix @@ -91,6 +92,7 @@ A step changes a direction's delay, jitter, loss, reorder or rate limit once the from when forwarding started. It changes only what it names, so a later step puts one knob back. A rate step keeps the old bucket's credit, or its debt, clipped to the new one, and a limit that was not there starts full. A step can add or change a rate limit, never remove one. Steps come from a profile file. +A run that ends before its last step fails `verify`, since that traffic never saw the path change. ## Profiles diff --git a/rs/moq-shaper/src/lib.rs b/rs/moq-shaper/src/lib.rs index 5b0e0f24ca..797d8568e6 100644 --- a/rs/moq-shaper/src/lib.rs +++ b/rs/moq-shaper/src/lib.rs @@ -439,20 +439,35 @@ impl Shaper { } } - /// Fail unless the shaper is still forwarding and every impairment the - /// profile configures acted on some datagram, in either direction. + /// Fail unless the shaper is still forwarding, every impairment the profile + /// configures acted on some datagram, and some datagram saw every step, in + /// either direction. /// /// An impairment is only held to that once the traffic makes its silence - /// implausible: a short run can see no loss, but never no delay. + /// implausible: a short run can see no loss, but never no delay. A step is + /// held to it once its direction carried anything: traffic that all came + /// before the step never saw the path change. pub fn verify(&self) -> anyhow::Result { let stats = self.stats(); if let Some(err) = self.failed.get() { anyhow::bail!("the shaper stopped forwarding: {err} ({stats})"); } - let mut missing = unapplied(&self.setup.config, &stats); + let mut missing: Vec = unapplied(&self.setup.config, &stats) + .into_iter() + .map(String::from) + .collect(); if unbatched(&self.setup, &stats) { - missing.push("batch"); + missing.push("batch".to_string()); + } + for (name, options, tally, counters) in [ + ("up", &self.setup.up, &self.tally[UP], &stats.up), + ("down", &self.setup.down, &self.tally[DOWN], &stats.down), + ] { + let reached = tally.stepped.load(Ordering::Relaxed) as usize; + if let Some(step) = options.steps.get(reached).filter(|_| counters.packets > 0) { + missing.push(format!("the {name} step at {:?}", step.at)); + } } anyhow::ensure!( missing.is_empty(), @@ -480,6 +495,8 @@ struct Tally { throttled: AtomicU64, delayed: AtomicU64, reordered: AtomicU64, + /// The most steps any one link has applied. + stepped: AtomicU64, } impl Tally { @@ -694,6 +711,11 @@ impl Held { /// Hold `parcel`, releasing the batch into `ready` once it is full. fn hold(&mut self, batch: Batch, parcel: Parcel, ready: &mut Vec, tally: &Tally) { let arrived = parcel.arrived; + // The window may have closed before its timer fired; the batch it closed + // leaves without this datagram, which starts the next one. + if let Some(closes) = self.closes.filter(|&closes| closes <= arrived) { + self.release(closes, ready, tally); + } self.closes.get_or_insert(arrived + batch.window); self.parcels.push(parcel); if self.parcels.len() >= batch.count { @@ -744,8 +766,10 @@ struct Link { floor: Instant, /// When the run started, which a step's `at` counts from. start: Instant, - /// The steps still to come, soonest last. + /// Every step, earliest first. steps: Vec, + /// How many of `steps` have applied. + stepped: usize, queue: mpsc::UnboundedSender, } @@ -772,21 +796,23 @@ impl Link { full_at: start, floor: start, start, - steps: options.steps.iter().rev().cloned().collect(), + steps: options.steps.clone(), + stepped: 0, queue, } } /// Apply every step the run has reached by `now`, in order. - fn step(&mut self, now: Instant) { - while self.steps.last().is_some_and(|step| self.start + step.at <= now) { - let step = self.steps.pop().expect("a step is due"); + fn step(&mut self, now: Instant, tally: &Tally) { + while let Some(step) = self.steps.get(self.stepped).filter(|step| self.start + step.at <= now) { if let Some(rate) = &step.rate { // Credit accrues at the new rate from the scheduled transition, // even when no datagram arrived at that instant. self.full_at = self.refilled(rate, self.start + step.at); } step.apply(&mut self.profile); + self.stepped += 1; + tally.stepped.fetch_max(self.stepped as u64, Ordering::Relaxed); } } @@ -822,7 +848,7 @@ impl Link { /// When a datagram of `size` bytes arriving `now` leaves, and whether it was /// counted as delayed, or `None` when it is dropped. fn treat(&mut self, now: Instant, size: usize, tally: &Tally) -> Option<(Instant, bool)> { - self.step(now); + self.step(now, tally); bump(&tally.packets); // Every draw happens for every datagram, whatever the profile, so one @@ -1480,6 +1506,16 @@ mod tests { held.release(closes, &mut ready, &tally); assert_eq!(ready.iter().map(|parcel| parcel.at).collect::>(), [closes]); assert_eq!(tally.snapshot().delayed, 3); + ready.clear(); + + // A datagram that arrives after the window closed, before its timer fired, + // starts the next batch rather than joining the closed one. + held.hold(batch, parcel(now + ms(200)), &mut ready, &tally); + let closes = held.closes.unwrap(); + held.hold(batch, parcel(closes + ms(1)), &mut ready, &tally); + assert_eq!(ready.iter().map(|parcel| parcel.at).collect::>(), [closes]); + assert_eq!(held.parcels.len(), 1, "the late datagram joined the closed batch"); + assert_eq!(held.closes, Some(closes + ms(1) + ms(160))); } #[tokio::test] @@ -1755,7 +1791,7 @@ mod tests { #[tokio::test] async fn a_step_gets_worse_part_way_through() { let ms = Duration::from_millis; - let (_shaper, client) = shaped(|config| Setup { + let (shaper, client) = shaped(|config| Setup { up: stepped(vec![Step { at: ms(150), delay: Some(ms(60)), @@ -1786,6 +1822,24 @@ mod tests { after > before + ms(30), "median latency went from {before:?} to {after:?}" ); + shaper.verify().unwrap(); + } + + #[tokio::test] + async fn a_step_the_run_never_reached_is_unapplied() { + let (shaper, client) = shaped(|config| Setup { + down: stepped(vec![Step { + at: Duration::from_secs(60), + delay: Some(Duration::from_millis(60)), + ..Default::default() + }]), + ..config.into() + }) + .await; + assert_eq!(round_trip(&client, 10).await, (0..10).collect::>()); + + let err = shaper.verify().expect_err("a run that ended before its step passed"); + assert!(format!("{err:#}").contains("the down step at 60s"), "{err:#}"); } #[tokio::test] From d9a9de7550504baffb4ff0384acb0790a3d784e9 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 29 Sep 2026 11:42:40 -0700 Subject: [PATCH 13/19] fix(shaper): refuse a step that changes nothing and a batch of one A step restating the value already in force left the profile unchanged yet counted as reached. A batch of one released every datagram on arrival. Both now fail before binding. Co-Authored-By: Claude Opus 5.5 --- rs/moq-shaper/README.md | 6 ++++-- rs/moq-shaper/src/lib.rs | 33 +++++++++++++++++++++------------ 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/rs/moq-shaper/README.md b/rs/moq-shaper/README.md index 7adfee3d35..9d2145cb9c 100644 --- a/rs/moq-shaper/README.md +++ b/rs/moq-shaper/README.md @@ -84,14 +84,16 @@ both. A flow's treatment then depends on how its datagrams interleave with the o A paced hop, a Wi-Fi access point or a cellular scheduler bunches traffic: nothing moves for a while, then several datagrams arrive at once. A batch holds datagrams until `count` are waiting, or until `window` after the first arrived, then sends them all when the latest would have left. No delay or jitter produces -that clump, and it is what a receiver's jitter estimate has to cope with. +that clump, and it is what a receiver's jitter estimate has to cope with. A `count` below two is refused, +since the datagram that fills a batch never waits. ## Steps A step changes a direction's delay, jitter, loss, reorder or rate limit once the run reaches `at`, counted from when forwarding started. It changes only what it names, so a later step puts one knob back. A rate step keeps the old bucket's credit, or its debt, clipped to the new one, and a limit that was not there -starts full. A step can add or change a rate limit, never remove one. Steps come from a profile file. +starts full. A step can add or change a rate limit, never remove one. A step that leaves the profile as +it was is refused. Steps come from a profile file. A run that ends before its last step fails `verify`, since that traffic never saw the path change. ## Profiles diff --git a/rs/moq-shaper/src/lib.rs b/rs/moq-shaper/src/lib.rs index 797d8568e6..dd7d27f63c 100644 --- a/rs/moq-shaper/src/lib.rs +++ b/rs/moq-shaper/src/lib.rs @@ -188,7 +188,8 @@ impl Options { fn validate(&self, profile: &Profile) -> anyhow::Result<()> { profile.validate(self.jitter_model)?; if let Some(batch) = &self.batch { - anyhow::ensure!(batch.count > 0, "a batch of zero datagrams never closes"); + // The datagram that fills a batch leaves at once, so one of one holds nothing. + anyhow::ensure!(batch.count > 1, "a batch of {} never holds anything", batch.count); anyhow::ensure!(!batch.window.is_zero(), "a batch with no window never holds anything"); } @@ -202,16 +203,10 @@ impl Options { "the step at {:?} comes after the one at {previous:?}; steps go in order", step.at ); - // A step naming nothing is a profile that thinks it changes and does not. - anyhow::ensure!( - Step { - at: step.at, - ..Step::default() - } != *step, - "the step at {:?} changes nothing", - step.at - ); + // A step that leaves the profile as it was thinks it changes and does not. + let before = profile.clone(); step.apply(&mut profile); + anyhow::ensure!(profile != before, "the step at {:?} changes nothing", step.at); profile .validate(self.jitter_model) .with_context(|| format!("after the step at {:?}", step.at))?; @@ -1612,8 +1607,9 @@ mod tests { .unwrap_or_else(|| panic!("accepted a batch where {why}")); assert!(format!("{err:#}").contains(why), "{err:#}"); }; - refused(batched(0, Duration::from_millis(10)), "never closes").await; - refused(batched(7, Duration::ZERO), "never holds").await; + refused(batched(0, Duration::from_millis(10)), "a batch of 0 never holds").await; + refused(batched(1, Duration::from_millis(10)), "a batch of 1 never holds").await; + refused(batched(7, Duration::ZERO), "no window never holds").await; } #[test] @@ -1879,6 +1875,19 @@ mod tests { "changes nothing", ) .await; + // Restating the value already in force changes nothing either. + refused( + vec![ + at(30), + Step { + at: Duration::from_secs(60), + loss: Some(0.1), + ..Default::default() + }, + ], + "the step at 60s changes nothing", + ) + .await; // Uniform jitter past the delay, reached by a step rather than at the start. refused( vec![Step { From 748447b53b97e764c030010739f699595f7be279 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 29 Sep 2026 12:00:56 -0700 Subject: [PATCH 14/19] fix(audio-quality): measure a step row from before its step, and count only batch holds The driver now gives the session and first audio one 20s deadline, short of the step profile's change at 30s, so a step row never grades only the post-step path. `verify` now proves a batch acted from a counter only batches bump, so a profile delay no longer passes a batch that never changed a departure. Co-Authored-By: Claude Opus 5.5 --- rs/moq-shaper/src/lib.rs | 56 +++++++++++++++++++------ test/audio-quality/README.md | 5 ++- test/audio-quality/clients/js/driver.ts | 11 +++-- 3 files changed, 53 insertions(+), 19 deletions(-) diff --git a/rs/moq-shaper/src/lib.rs b/rs/moq-shaper/src/lib.rs index dd7d27f63c..1bcfd2fa6f 100644 --- a/rs/moq-shaper/src/lib.rs +++ b/rs/moq-shaper/src/lib.rs @@ -253,13 +253,14 @@ fn unapplied(config: &Config, stats: &Stats) -> Vec<&'static str> { /// Whether the batches `setup` configures implausibly never held a datagram. /// /// A batch holds every datagram but the one that fills it, so it acts on at -/// least `1 - 1 / count` of them. What it holds counts as delayed once the -/// batch leaves. -fn unbatched(setup: &Setup, stats: &Stats) -> bool { +/// least `1 - 1 / count` of them. `batched` counts only what a batch held, not +/// what the profile delayed, so a batch that never changed a departure is caught +/// even behind a delay. +fn unbatched(setup: &Setup, stats: &Stats, batched: u64) -> bool { let chance = |options: &Options| options.batch.map_or(0.0, |batch| 1.0 - 1.0 / batch.count as f64); let silence = (1.0 - chance(&setup.up)).powf(stats.up.packets as f64) * (1.0 - chance(&setup.down)).powf(stats.down.packets as f64); - stats.up.delayed + stats.down.delayed == 0 && silence < IMPLAUSIBLE + batched == 0 && silence < IMPLAUSIBLE } /// A token-bucket rate limit with a bounded queue behind it. @@ -452,7 +453,12 @@ impl Shaper { .into_iter() .map(String::from) .collect(); - if unbatched(&self.setup, &stats) { + let batched = self + .tally + .iter() + .map(|tally| tally.batched.load(Ordering::Relaxed)) + .sum(); + if unbatched(&self.setup, &stats, batched) { missing.push("batch".to_string()); } for (name, options, tally, counters) in [ @@ -492,6 +498,8 @@ struct Tally { reordered: AtomicU64, /// The most steps any one link has applied. stepped: AtomicU64, + /// Datagrams a batch held past when they would otherwise have left. + batched: AtomicU64, } impl Tally { @@ -727,9 +735,12 @@ impl Held { }; let at = latest.max(closed); for mut parcel in self.parcels.drain(..) { - // A hold is a delay, counted once whichever stage gave it. - if at > parcel.at && !parcel.delayed { - bump(&tally.delayed); + if at > parcel.at { + bump(&tally.batched); + // A hold is a delay, counted once whichever stage gave it. + if !parcel.delayed { + bump(&tally.delayed); + } } parcel.at = at; ready.push(parcel); @@ -1511,6 +1522,23 @@ mod tests { assert_eq!(ready.iter().map(|parcel| parcel.at).collect::>(), [closes]); assert_eq!(held.parcels.len(), 1, "the late datagram joined the closed batch"); assert_eq!(held.closes, Some(closes + ms(1) + ms(160))); + assert_eq!(tally.snapshot().delayed, 4); + assert_eq!(tally.batched.load(Ordering::Relaxed), 4); + + // Behind a delay longer than the window, a lone datagram leaves when the + // delay says: the batch held nothing, whatever `delayed` counts. + ready.clear(); + let mut held = Held::default(); + let tally = Tally::default(); + let late = Parcel { + at: now + ms(1000), + delayed: true, + ..parcel(now) + }; + held.hold(batch, late, &mut ready, &tally); + held.release(held.closes.unwrap(), &mut ready, &tally); + assert_eq!(ready[0].at, now + ms(1000)); + assert_eq!(tally.batched.load(Ordering::Relaxed), 0); } #[tokio::test] @@ -1634,13 +1662,15 @@ mod tests { }; // (1/7)^2 is 2%: two datagrams through batches of seven prove nothing. - assert!(!unbatched(&setup, &quiet(2))); + assert!(!unbatched(&setup, &quiet(2), 0)); // (1/7)^10 is 4e-9: ten datagrams a batch never held means no batch. - assert!(unbatched(&setup, &quiet(10))); + assert!(unbatched(&setup, &quiet(10), 0)); + assert!(!unbatched(&setup, &quiet(10), 6)); - let mut held = quiet(10); - held.up.delayed = 6; - assert!(!unbatched(&setup, &held)); + // A delay the profile gave is not a batch holding anything. + let mut delayed = quiet(10); + delayed.up.delayed = 10; + assert!(unbatched(&setup, &delayed, 0)); } /// How long after `at` each of `sizes` leaves one link, fed at the same instant. diff --git a/test/audio-quality/README.md b/test/audio-quality/README.md index c9640eb993..c64b180aeb 100644 --- a/test/audio-quality/README.md +++ b/test/audio-quality/README.md @@ -44,7 +44,8 @@ fixed preset regressed, and a fixed preset is what many viewers land on. Loss, r limits stay off: the buffer's job is absorbing arrival spread, and congestion response would make a failure hard to attribute. The `mild` and `wide` profiles preserve datagram order and share one path across the page's sessions. `bursty` and `step` use the shaper's named profiles on that same shared path. The step is timed from -shaper startup, before the page connects; `--duration` must exceed 30 s so playback spans it. +shaper startup, before the page connects; the driver gives up on a row with no audio 20 s in, so the +measured window starts before the step, and `--duration` must exceed 30 s so playback spans it. The AAC arm keeps ffmpeg's default PES packing, whose multi-frame bursts are the flush-span shape the reporter measured on the public relay (#3477). The source is a sine tone rather than a film, so a @@ -140,7 +141,7 @@ A row that cannot be trusted is void, reported rather than graded, and fails an | `ring` | `crossOriginIsolated` does not match the row's ring, so the other ring ran. | | `clock` | The AudioContext clock was unreadable or ran more than 1% off the viewer's. | | `window` | No audio after the warmup. | -| `driver` | The page never reached a session or first audio within 30 s, or the driver threw. | +| `driver` | The page never reached a session and first audio within 20 s, or the driver threw. | ## Budgets diff --git a/test/audio-quality/clients/js/driver.ts b/test/audio-quality/clients/js/driver.ts index 3fec53e203..e640a584f9 100644 --- a/test/audio-quality/clients/js/driver.ts +++ b/test/audio-quality/clients/js/driver.ts @@ -57,8 +57,12 @@ if ( process.exit(2); } -/** How long the session and the first audio each get before the row is abandoned. */ -const STARTUP_MS = 30_000; +/** + * How long the session and the first audio get, together, before the row is abandoned. It stays short + * of the step profile's change at 30s, so a step row always measures audio from before the change. + */ +const STARTUP_MS = 20_000; +const startupDeadline = Date.now() + STARTUP_MS; /** How often the probe is drained into the ndjson. */ const DRAIN_MS = 1000; @@ -98,8 +102,7 @@ const refuse = (assertion: string, detail: string) => { /** Poll the page until `ready` returns a value, or throw naming what never happened. */ async function waitFor(page: Page, what: string, ready: () => T | undefined): Promise { - const deadline = Date.now() + STARTUP_MS; - while (Date.now() < deadline) { + while (Date.now() < startupDeadline) { const value = await page.evaluate(ready); if (value !== undefined && value !== null) return value; await sleep(200); From 2ecb0abecc611eafbdbdc5007cdcb728a61db310 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 29 Sep 2026 12:43:31 -0700 Subject: [PATCH 15/19] fix(shaper): refuse jitter with no delay to vary A gaussian profile with jitter but no delay passed `verify` even when every draw clamped to zero, since a zero delay has no chance to act. The uniform model already refused it as jitter exceeding delay. Co-Authored-By: Claude Opus 5.5 --- rs/moq-shaper/README.md | 2 +- rs/moq-shaper/src/lib.rs | 28 ++++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/rs/moq-shaper/README.md b/rs/moq-shaper/README.md index 9d2145cb9c..b7d8001184 100644 --- a/rs/moq-shaper/README.md +++ b/rs/moq-shaper/README.md @@ -64,7 +64,7 @@ later datagram can overtake an earlier one. A drill wants that, and `jitter` may `gaussian` treats `jitter` as a sigma, clamps the draw at zero, and never lets a datagram leave before the one in front of it: queueing delay on a FIFO path, which stretches and compresses the spacing and never -changes the order. Only `reorder` overtakes. This matters beyond realism: QUIC reads an overtake as loss, +changes the order. The sigma may exceed `delay`, but a nonzero `jitter` still needs a nonzero `delay`. Only `reorder` overtakes. This matters beyond realism: QUIC reads an overtake as loss, retransmits and backs off, so a run meant to measure a jittery path ends up measuring the congestion response. On a 5ms delay with 5ms of jitter, reordering alone dragged a receiver's audio buffer from 120ms to nearly two seconds. diff --git a/rs/moq-shaper/src/lib.rs b/rs/moq-shaper/src/lib.rs index 1bcfd2fa6f..8e1bdf9cfa 100644 --- a/rs/moq-shaper/src/lib.rs +++ b/rs/moq-shaper/src/lib.rs @@ -83,6 +83,12 @@ impl Profile { self.jitter, self.delay ); + // With no delay to vary, `verify` has nothing to hold the jitter to. + anyhow::ensure!( + self.jitter.is_zero() || !self.delay.is_zero(), + "jitter {:?} needs a delay to vary", + self.jitter + ); if let Some(rate) = &self.rate { anyhow::ensure!(rate.bits_per_second > 0, "a rate limit of zero passes nothing"); } @@ -1261,6 +1267,7 @@ mod tests { fn gaussian_jitter_never_leaves_before_it_arrived() { // A sigma far past the delay, so most draws would be negative unclamped. let wide = Profile { + delay: Duration::from_millis(1), jitter: Duration::from_millis(50), ..Default::default() }; @@ -1355,6 +1362,27 @@ mod tests { .await; assert_eq!(round_trip(&client, 20).await, (0..20).collect::>()); shaper.verify().unwrap(); + + // With no delay, `verify` could never tell the jitter acted. + let undelayed = Profile { + jitter: Duration::from_millis(5), + ..Default::default() + }; + let err = Shaper::bind(Setup { + up: GAUSSIAN, + ..Config { + bind: LOCALHOST, + target: LOCALHOST, + seed: 0, + up: undelayed, + down: Profile::default(), + } + .into() + }) + .await + .err() + .expect("accepted gaussian jitter with no delay"); + assert!(format!("{err:#}").contains("needs a delay to vary"), "{err:#}"); } /// Two clients take turns sending `count` numbered datagrams up the path From 94c34831db1d993ceb72fd8844723c91f5cdb9c2 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 29 Sep 2026 16:13:30 -0700 Subject: [PATCH 16/19] test(audio-quality): count browser rows apart from replays; size the questline Co-Authored-By: Claude Opus 5.5 --- quest/m0/audio-quality-harness/README.md | 2 +- test/audio-quality/clients/js/src/run.test.ts | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/quest/m0/audio-quality-harness/README.md b/quest/m0/audio-quality-harness/README.md index 2fff2083b9..42ac07ff3d 100644 --- a/quest/m0/audio-quality-harness/README.md +++ b/quest/m0/audio-quality-harness/README.md @@ -1,4 +1,4 @@ -# Audio quality harness +# [S] Audio quality harness ## Goal diff --git a/test/audio-quality/clients/js/src/run.test.ts b/test/audio-quality/clients/js/src/run.test.ts index 7cd6a67d10..13bda94a13 100644 --- a/test/audio-quality/clients/js/src/run.test.ts +++ b/test/audio-quality/clients/js/src/run.test.ts @@ -22,7 +22,11 @@ test("short steady rows remain supported", () => { test("the nightly matrix includes both new profiles", () => { const result = Bun.spawnSync(["bash", script, "--list"]); expect(result.exitCode).toBe(0); - const rows = result.stdout.toString().trim().split("\n"); + const rows = result.stdout + .toString() + .trim() + .split("\n") + .filter((row) => row.startsWith("chromium-")); expect(rows).toHaveLength(24); for (const profile of ["bursty", "step"]) { expect(rows.filter((row) => row.includes(`-${profile}-`))).toHaveLength(4); From b7a48e6f2fcd8fb898e4a40ba8b895a93e9d6082 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 30 Sep 2026 21:40:20 -0700 Subject: [PATCH 17/19] fix(moq-shaper): verify every step's phase saw traffic A step counted as reached when it applied, so a quiet stretch could skip an intermediate phase and still pass verify. Record the phase each link treats a datagram in and require each step's. Refuse a step at zero, and treat a step past what the clock can hold as never reached instead of panicking. Co-Authored-By: Claude Opus 5.5 --- rs/moq-shaper/README.md | 7 +-- rs/moq-shaper/src/lib.rs | 97 +++++++++++++++++++++++++++++++++++----- 2 files changed, 91 insertions(+), 13 deletions(-) diff --git a/rs/moq-shaper/README.md b/rs/moq-shaper/README.md index b7d8001184..0a034b4e63 100644 --- a/rs/moq-shaper/README.md +++ b/rs/moq-shaper/README.md @@ -45,7 +45,7 @@ As a library, `Shaper::bind` takes a `Config`: where to listen and forward, the per direction. It also takes a `Setup`, which is a `Config` plus the opt-in options below, each off by default. `Shaper::verify` fails when the shaper stopped forwarding, when an impairment the profile configures never acted and the traffic makes that silence implausible, or when a direction carried -traffic but none of it reached a step. The relay's drills +traffic but none of it saw some step's profile. The relay's drills (`rs/moq-relay/tests/drills.rs`, described in `test/drill/README.md`) run every scenario through it. ## What a seed does and does not fix @@ -93,8 +93,9 @@ A step changes a direction's delay, jitter, loss, reorder or rate limit once the from when forwarding started. It changes only what it names, so a later step puts one knob back. A rate step keeps the old bucket's credit, or its debt, clipped to the new one, and a limit that was not there starts full. A step can add or change a rate limit, never remove one. A step that leaves the profile as -it was is refused. Steps come from a profile file. -A run that ends before its last step fails `verify`, since that traffic never saw the path change. +it was, or comes at zero, is refused. Steps come from a profile file. +A run where no datagram saw some step's profile fails `verify`, whether it ended before the step or +went quiet until a later step replaced it. ## Profiles diff --git a/rs/moq-shaper/src/lib.rs b/rs/moq-shaper/src/lib.rs index 8e1bdf9cfa..d46a0785ea 100644 --- a/rs/moq-shaper/src/lib.rs +++ b/rs/moq-shaper/src/lib.rs @@ -20,7 +20,7 @@ use std::{ cmp::Reverse, - collections::{BinaryHeap, HashMap, hash_map}, + collections::{BTreeSet, BinaryHeap, HashMap, hash_map}, fmt, net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}, sync::{ @@ -202,6 +202,9 @@ impl Options { let mut profile = profile.clone(); let mut previous: Option = None; for step in &self.steps { + // The profile is what the run opens with, so a step at zero would + // replace it before any datagram saw it. + anyhow::ensure!(!step.at.is_zero(), "a step at zero replaces the profile; change the profile instead"); // A link only ever looks at the next step due, so one out of order // would be skipped without a word. anyhow::ensure!( @@ -471,9 +474,14 @@ impl Shaper { ("up", &self.setup.up, &self.tally[UP], &stats.up), ("down", &self.setup.down, &self.tally[DOWN], &stats.down), ] { - let reached = tally.stepped.load(Ordering::Relaxed) as usize; - if let Some(step) = options.steps.get(reached).filter(|_| counters.packets > 0) { - missing.push(format!("the {name} step at {:?}", step.at)); + if counters.packets == 0 { + continue; + } + let phases = tally.phases.lock().unwrap(); + for (index, step) in options.steps.iter().enumerate() { + if !phases.contains(&(index + 1)) { + missing.push(format!("the {name} step at {:?}", step.at)); + } } } anyhow::ensure!( @@ -502,8 +510,8 @@ struct Tally { throttled: AtomicU64, delayed: AtomicU64, reordered: AtomicU64, - /// The most steps any one link has applied. - stepped: AtomicU64, + /// The phases some link treated a datagram in: 0 before the first step, n after the nth. + phases: Mutex>, /// Datagrams a batch held past when they would otherwise have left. batched: AtomicU64, } @@ -782,6 +790,8 @@ struct Link { steps: Vec, /// How many of `steps` have applied. stepped: usize, + /// Whether a datagram was treated since the latest step applied. + treated: bool, queue: mpsc::UnboundedSender, } @@ -810,21 +820,32 @@ impl Link { start, steps: options.steps.clone(), stepped: 0, + treated: false, queue, } } - /// Apply every step the run has reached by `now`, in order. + /// Apply every step the run has reached by `now`, in order, and record the + /// phase a datagram arriving then sees. fn step(&mut self, now: Instant, tally: &Tally) { - while let Some(step) = self.steps.get(self.stepped).filter(|step| self.start + step.at <= now) { + while let Some(step) = self.steps.get(self.stepped) { + // A step past what the clock can hold is one the run never reaches. + let Some(at) = self.start.checked_add(step.at).filter(|at| *at <= now) else { + break; + }; if let Some(rate) = &step.rate { // Credit accrues at the new rate from the scheduled transition, // even when no datagram arrived at that instant. - self.full_at = self.refilled(rate, self.start + step.at); + self.full_at = self.refilled(rate, at); } step.apply(&mut self.profile); self.stepped += 1; - tally.stepped.fetch_max(self.stepped as u64, Ordering::Relaxed); + self.treated = false; + } + // Applying a step is no evidence it acted, so record the phase a datagram sees. + if !self.treated { + self.treated = true; + tally.phases.lock().unwrap().insert(self.stepped); } } @@ -1896,6 +1917,61 @@ mod tests { assert!(format!("{err:#}").contains("the down step at 60s"), "{err:#}"); } + #[tokio::test(start_paused = true)] + async fn a_step_no_datagram_saw_is_unapplied() { + let ms = Duration::from_millis; + let (shaper, client) = shaped(|config| Setup { + up: stepped(vec![ + Step { + at: ms(100), + delay: Some(ms(60)), + ..Default::default() + }, + Step { + at: ms(200), + delay: Some(ms(5)), + ..Default::default() + }, + ]), + ..Config { + up: Profile { + delay: ms(5), + ..Default::default() + }, + ..config + } + .into() + }) + .await; + + // One datagram before the first step and one after the second, so the + // 60ms phase passes with nothing in it. + let mut buf = [0u8; 4]; + client.send(&0u32.to_be_bytes()).await.unwrap(); + client.recv(&mut buf).await.unwrap(); + tokio::time::sleep(ms(250)).await; + client.send(&1u32.to_be_bytes()).await.unwrap(); + client.recv(&mut buf).await.unwrap(); + + let err = shaper.verify().expect_err("a run that skipped a step passed"); + let err = format!("{err:#}"); + assert!(err.contains("the up step at 100ms"), "{err}"); + assert!(!err.contains("the up step at 200ms"), "{err}"); + } + + #[test] + fn a_step_past_the_clock_never_applies() { + let options = stepped(vec![Step { + at: Duration::MAX, + delay: Some(Duration::from_millis(60)), + ..Default::default() + }]); + let (queue, _) = mpsc::unbounded_channel(); + let start = Instant::now(); + let mut link = Link::new(&Profile::default(), &options, 7, 0, start, queue); + assert_eq!(owed(&mut link, start + Duration::from_secs(1), &[16]), [Duration::ZERO]); + } + #[tokio::test] async fn steps_that_would_be_skipped_or_do_nothing_are_refused() { let refused = |steps: Vec, why: &'static str| async move { @@ -1925,6 +2001,7 @@ mod tests { }; refused(vec![at(60), at(30)], "steps go in order").await; + refused(vec![at(0)], "a step at zero").await; refused( vec![Step { at: Duration::from_secs(30), From 01ab236d95868dd85f7e9977975c58f152d6f8c4 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 30 Sep 2026 21:47:24 -0700 Subject: [PATCH 18/19] style(moq-shaper): format Co-Authored-By: Claude Opus 5.5 --- rs/moq-shaper/src/lib.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/rs/moq-shaper/src/lib.rs b/rs/moq-shaper/src/lib.rs index d46a0785ea..8755d387e6 100644 --- a/rs/moq-shaper/src/lib.rs +++ b/rs/moq-shaper/src/lib.rs @@ -204,7 +204,10 @@ impl Options { for step in &self.steps { // The profile is what the run opens with, so a step at zero would // replace it before any datagram saw it. - anyhow::ensure!(!step.at.is_zero(), "a step at zero replaces the profile; change the profile instead"); + anyhow::ensure!( + !step.at.is_zero(), + "a step at zero replaces the profile; change the profile instead" + ); // A link only ever looks at the next step due, so one out of order // would be skipped without a word. anyhow::ensure!( From beeb93d18cc82fd1d9c0667da29208e8973a1734 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 30 Sep 2026 23:18:40 -0700 Subject: [PATCH 19/19] fix(moq-shaper): require traffic before the first step too A direction whose traffic started after its first step never saw the transition, yet verify passed. Require the phase before the first step alongside each step's. Co-Authored-By: Claude Opus 5.5 --- rs/moq-shaper/README.md | 7 +++--- rs/moq-shaper/src/lib.rs | 49 ++++++++++++++++++++++++++++++++++------ 2 files changed, 46 insertions(+), 10 deletions(-) diff --git a/rs/moq-shaper/README.md b/rs/moq-shaper/README.md index 0a034b4e63..af9e215f0a 100644 --- a/rs/moq-shaper/README.md +++ b/rs/moq-shaper/README.md @@ -45,7 +45,7 @@ As a library, `Shaper::bind` takes a `Config`: where to listen and forward, the per direction. It also takes a `Setup`, which is a `Config` plus the opt-in options below, each off by default. `Shaper::verify` fails when the shaper stopped forwarding, when an impairment the profile configures never acted and the traffic makes that silence implausible, or when a direction carried -traffic but none of it saw some step's profile. The relay's drills +traffic but no datagram saw one of its phases, before the first step or after one. The relay's drills (`rs/moq-relay/tests/drills.rs`, described in `test/drill/README.md`) run every scenario through it. ## What a seed does and does not fix @@ -94,8 +94,9 @@ from when forwarding started. It changes only what it names, so a later step put step keeps the old bucket's credit, or its debt, clipped to the new one, and a limit that was not there starts full. A step can add or change a rate limit, never remove one. A step that leaves the profile as it was, or comes at zero, is refused. Steps come from a profile file. -A run where no datagram saw some step's profile fails `verify`, whether it ended before the step or -went quiet until a later step replaced it. +A direction that carried traffic fails `verify` unless some datagram saw each phase: the profile before +the first step and after every step. Traffic that starts late, ends early, or goes quiet across a step +never saw the path change. ## Profiles diff --git a/rs/moq-shaper/src/lib.rs b/rs/moq-shaper/src/lib.rs index 8755d387e6..5da562780e 100644 --- a/rs/moq-shaper/src/lib.rs +++ b/rs/moq-shaper/src/lib.rs @@ -448,13 +448,13 @@ impl Shaper { } /// Fail unless the shaper is still forwarding, every impairment the profile - /// configures acted on some datagram, and some datagram saw every step, in - /// either direction. + /// configures acted on some datagram, and some datagram saw every phase of + /// a direction with steps, before the first and after each. /// /// An impairment is only held to that once the traffic makes its silence - /// implausible: a short run can see no loss, but never no delay. A step is - /// held to it once its direction carried anything: traffic that all came - /// before the step never saw the path change. + /// implausible: a short run can see no loss, but never no delay. The phases + /// are held to it once their direction carried anything: traffic that all + /// came on one side of a step never saw the path change. pub fn verify(&self) -> anyhow::Result { let stats = self.stats(); if let Some(err) = self.failed.get() { @@ -477,10 +477,13 @@ impl Shaper { ("up", &self.setup.up, &self.tally[UP], &stats.up), ("down", &self.setup.down, &self.tally[DOWN], &stats.down), ] { - if counters.packets == 0 { + let Some(first) = options.steps.first().filter(|_| counters.packets > 0) else { continue; - } + }; let phases = tally.phases.lock().unwrap(); + if !phases.contains(&0) { + missing.push(format!("the {name} profile before its step at {:?}", first.at)); + } for (index, step) in options.steps.iter().enumerate() { if !phases.contains(&(index + 1)) { missing.push(format!("the {name} step at {:?}", step.at)); @@ -1962,6 +1965,38 @@ mod tests { assert!(!err.contains("the up step at 200ms"), "{err}"); } + #[tokio::test(start_paused = true)] + async fn a_profile_no_datagram_saw_before_its_step_is_unapplied() { + let ms = Duration::from_millis; + let (shaper, client) = shaped(|config| Setup { + up: stepped(vec![Step { + at: ms(100), + delay: Some(ms(60)), + ..Default::default() + }]), + ..Config { + up: Profile { + delay: ms(5), + ..Default::default() + }, + ..config + } + .into() + }) + .await; + + // The first datagram comes after the step, so the 5ms phase passes with nothing in it. + tokio::time::sleep(ms(250)).await; + let mut buf = [0u8; 4]; + client.send(&0u32.to_be_bytes()).await.unwrap(); + client.recv(&mut buf).await.unwrap(); + + let err = shaper.verify().expect_err("a run that started after its step passed"); + let err = format!("{err:#}"); + assert!(err.contains("the up profile before its step at 100ms"), "{err}"); + assert!(!err.contains("the up step at"), "{err}"); + } + #[test] fn a_step_past_the_clock_never_applies() { let options = stepped(vec![Step {