Skip to content

feat(congestion)!: BBR ships through the selector, candidates move to moq-congestion-lab - #8850

Open
kixelated wants to merge 4 commits into
mainfrom
quest/m2/congestion-candidates
Open

kixelated wants to merge 4 commits into
mainfrom
quest/m2/congestion-candidates

Conversation

@kixelated

@kixelated kixelated commented Nov 15, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

moq-congestion 0.1.0 has never been published, yet its public API carried three candidates no session can select: GCC (gcc, about 16k vendored WebRTC lines under BSD), SCReAMv2 (scream), and BBR3's delay bound (bbr3::Config::delay_bound). The maintainer's pick on moq-dev/moq#8764: ship BBR through one selector, keep the candidates off the published API until tested, and make new algorithms additive. moq-ffi's MoqCongestionControl was a closed enum, so a new algorithm would break every Swift, Kotlin, and Dart caller that switches over it.

Completes quest/m2/congestion-candidates.md.

Approach

  • GCC and SCReAMv2 move unchanged to a new workspace-only crate, rs/moq-congestion-lab (publish = false). moq-quic's sim comparisons and moq-tokio's real-socket SCReAM tests take it as a path dev-dependency, which cargo publish drops. It gets its own tests (the large-MTU window check), and the nightly rust-test bench group now includes it.
  • #[doc(hidden)] is banned (sh/text.sh), so what the lab needs from moq-congestion sits behind a documented test-support feature, lab, the way sim and fuzz do elsewhere: the pacer module, a packet's GCC probe-cluster accessors, and the delay-bound setter. Without lab, the probe-cluster field isn't compiled.
  • The selector stays moq_tokio::quic::CongestionControl (#[non_exhaustive], relay/CLI congestion_control). Its docs now say plainly that delay is BBR v3 and the default, and that a new algorithm is added as a variant named after it.
  • MoqCongestionControl becomes an immutable uniffi object with loss(), delay(), and none() constructors, following the MoqAudioCodec precedent, with Eq/Hash/Debug exported. A new algorithm is a new constructor, which breaks no binding. Wrappers, tests, and docs are updated in Swift, Kotlin, Dart (bindings regenerated), Python, Go, C++ (OBS plugin), and moq-ffi.
  • Quests that point at the moved code follow it (vendor-sync scream/webrtc, webrtc README, quic-realtime). quic-realtime's stale "add Realtime" naming item is aligned with the newer decision to name each winner after its algorithm.

Impact

  • moq-congestion (unpublished): removes gcc and scream (now moq_congestion_lab::{gcc, scream}, workspace-only). bbr3::Config::delay_bound now requires the new lab feature, which also exposes pacer and Packet::{probe_cluster, set_probe_cluster} as test support. The license is now MIT OR Apache-2.0 (the BSD-3-Clause code left with GCC).
  • moq-tokio: quic::CongestionControl also derives Hash (additive). Docs only otherwise.
  • moq-ffi (breaking): MoqCongestionControl is an object instead of an enum. MoqQuicConfig.congestion_control is Option<Arc<MoqCongestionControl>>. Migration:
    • Swift: .delay / .loss / .none -> CongestionControl.delay() / .loss() / .none()
    • Kotlin: CongestionControl.DELAY -> CongestionControl.delay()
    • Dart: CongestionControl.delay -> CongestionControl.delay()
    • Python: CongestionControl.DELAY -> CongestionControl.delay()
    • Go: the CongestionControlLoss/Delay/None constants are gone; use moq.LossCongestionControl(), moq.DelayCongestionControl(), moq.NoneCongestionControl(), assigned directly (no ptr(...))
    • C++: moq::CongestionControl::kDelay -> moq::CongestionControl::delay(); compare with eq()
    • C API: unchanged (it already takes the loss/delay/none string)
  • Relay/CLI config: unchanged (delay, loss, none).
  • Wire: none.

Choices made without the maintainer (for review)

  • Crate name moq-congestion-lab.
  • A documented lab feature, not #[doc(hidden)] (banned): the Goal's "not reachable from any published crate's public API" is met for applications, but the setter, pacer, and probe-cluster accessors can still be reached through an opt-in test-support feature, the same rule moq-dev/moq#8392 set for sim/fuzz. Alternatives rejected: deleting the delay-bound sim tests (loses the bake-off's harness), or compiling pacer.rs into the lab via #[path] (still needs a feature for the other two).
  • Dropped BSD-3-Clause from moq-congestion's license: the vendored WebRTC files and their LICENSE/PATENTS moved to the lab. bbr3/delay.rs reimplements GoogCC's trend detector in its own structure, keeping only its constants. If a reviewer reads it as a derivative work, restore the expression and copy the notice beside it.
  • MoqCongestionControl as an object (the MoqAudioCodec shape) rather than wrapper-only types in Swift/Kotlin/Dart, so every binding grows the same way and the API names match across languages.
  • Nightly benchmark series for GCC move from rust-test/moq-congestion/gcc/... to rust-test/moq-congestion-lab/gcc/..., so they start a new history.

Validation

  • just check (changed scope: rs incl. tests, go, kt, dart, cpp, obs_compile, c, python, text, nix, quest): passes.
  • just rs test -p moq-congestion -p moq-congestion-lab -p moq-quic: 1134 passed, including delay_bound_keeps_a_shorter_queue, rises_to_target_under_delay_bound, and the lab's GCC/SCReAM tests; moq-tokio SCReAM and moq-ffi config tests (client_connects_with_every_knob_set, rung) pass.
  • just py test (128 passed), just go check (wrapper tests incl. reconnect with LossCongestionControl()), just kt check, just dart check, and the OBS settings test, all on the new constructors. Swift wasn't run here (no toolchain on Linux); CI's macOS lane covers it.
  • cargo package --list -p moq-congestion contains no gcc/ or scream.rs; cargo package -p moq-congestion-lab refuses (publish = false).
  • The GCC per-packet benchmark runs from its new crate (just rs test --release -p moq-congestion-lab -E 'test(benchmark_per_packet)' --run-ignored only).
  • cargo doc with -D warnings passes for moq-congestion with and without lab.

Alternatives

Follow-ups

None.

Models

Size: S
Implementer: Claude Code, claude-opus-5-5, unknown effort (arm A)

🤖 Generated with Claude Code

(Written by Claude Opus 5.5)

Review

Rewritten by quest review after each round.

  • Head: 7678090

  • Base: origin/main at d09ca93

  • Risk: medium, the base has no RISK.md to name what is always high or customer-facing, so every change is medium

  • Classified: medium by default

  • Reviewer: Codex, gpt-6.1-sol, medium effort

  • Session: 01a1dfcc-cfd7-75e1-8793-d48ad864e19e, round 1

  • Score: 9/10, The controller extraction and binding updates preserve behavior with no actionable defects identified, though the source-only review did not include builds or tests.

  • Rounds: 1; first round 0; findings 0: fixed 0, refuted 0, open 0

  • Findings: none

kixelated and others added 4 commits November 15, 2026 06:39
…moq-congestion-lab

GCC and SCReAMv2 move to the workspace-only moq-congestion-lab crate, BBR3's
delay bound gets a hidden setter, and moq-ffi's MoqCongestionControl becomes
an object with constructors so a new algorithm is additive in every binding.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ort feature

doc(hidden) is banned, so the pacer, a packet's GCC probe cluster, and
BBR3's delay bound sit behind moq-congestion's documented lab feature, as
test support does elsewhere.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…w the move

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated kixelated changed the title The congestion selector ships BBR, and new algorithms join it additively feat(congestion)!: BBR ships through the selector, candidates move to moq-congestion-lab Nov 15, 2026
@kixelated
kixelated marked this pull request as ready for review November 15, 2026 15:22

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No blocking issues. One small doc gap is noted below.

Reviewed changes

I reviewed all four commits: the candidates move into a workspace-only crate, the lab test-support feature, and the conversion of the FFI congestion selector from an enum to an object in every binding.

  • moq-congestion-lab crate: GCC and SCReAMv2 move over unchanged except for imports and the probe_cluster accessors. The crate is publish = false, and moq-quic and moq-tokio take it only as a path dev-dependency. It has its own large-MTU window test.
  • lab feature on moq-congestion: this feature gates pub mod pacer, the Packet probe-cluster field and its accessors, and the bbr3::Config::delay_bound setter. The license drops BSD-3-Clause, and the unused tracing dependency is removed.
  • MoqCongestionControl as a uniffi object: it has loss(), delay() and none() constructors and exports Debug/Eq/Hash. MoqQuicConfig.congestion_control becomes Option<Arc<_>>. Dart is regenerated. Go gets builder functions and a generic optional[T]. Kotlin, Python, Swift, OBS C++ and the tests switch to the constructors.
  • Selector docs: moq_tokio::quic::CongestionControl now derives Hash. The docs in the relay config, the C API and the bindings say delay is BBR v3 and the default.
  • Nightly and quests: the rust-test group adds -p moq-congestion-lab, so the GCC benchmark_per_packet still runs and still matches its filter. The completed quest is removed, and the vendor-sync and quic-realtime quests point at the new paths.

I checked locally: cargo check --all-targets passes with no warnings for moq-congestion without lab, moq-congestion with --features lab, and moq-congestion-lab. A grep finds the old enum spellings (CongestionControl.DELAY, kLoss, CongestionControlLoss, and so on) only in historical MIGRATE.md entries.

ℹ️ doc/setup/repository.md doesn't list moq-congestion-lab

AGENTS.md sends contributors and agents to doc/setup/repository.md to find where a feature lives. Its "Congestion controllers and controller tests" row still lists only rs/moq-congestion. After this PR, someone looking for GCC or SCReAMv2 there will look in the wrong crate.

Technical details
# repository.md omits the new lab crate

## Affected sites
- doc/setup/repository.md:21 — the "Congestion controllers and controller tests" row links only `rs/moq-congestion`

## Required outcome
- The row also points to `rs/moq-congestion-lab` as the home of the candidate controllers under test (GCC, SCReAMv2)

Pullfrog  | Fix it ➔ | View workflow run | Using claude-opus-5-5 | 𝕏

@coderabbitai

coderabbitai Bot commented Nov 15, 2026

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The changes add moq-congestion-lab for GCC and SCReAMv2 candidates and restrict related support in moq-congestion with the lab feature. The Rust FFI replaces its congestion-control enum with a native-backed object, and the Dart bindings add corresponding native handle operations. Callers and documentation across several language bindings are updated to use congestion-control constructors or builders. Rust test dependencies and candidate-controller references are updated.

Merge Risk: Minimal · up to 76780

This change moves experimental GCC and SCReAM controllers into an unpublished lab crate and makes the FFI congestion-control setting an object with loss(), delay() and none() constructors. The binding callers and documentation are updated to match. The BBR-based delay selection, the relay configuration and the wire format are unchanged. No concrete merge-blocking defect was found; the API change is an intentional breaking change, and CI is the remaining check for the generated Go and Swift bindings.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title names the main change: BBR ships through the existing congestion-control selector, and the candidate algorithms (GCC, SCReAMv2) move to the new workspace-only moq-congestion-lab crate. The r…
Description check Passed The description is closely related to the changeset. It explains the problem (unpublished GCC, SCReAMv2, and delay-bound candidates in the moq-congestion API), the approach (new moq-congestion-lab cra…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title names the main change: BBR ships through the existing congestion-control selector, and the candidate algorithms (GCC, SCReAMv2) move to the new workspace-only moq-congestion-lab crate. The raw summary confirms this matches the changeset: moq-congestion drops gcc/scream, the lab crate is added, and the selector docs now describe delay as BBR v3.

Full details: Description check

Explanation

The description is closely related to the changeset. It explains the problem (unpublished GCC, SCReAMv2, and delay-bound candidates in the moq-congestion API), the approach (new moq-congestion-lab crate, a documented lab feature, the MoqCongestionControl object change), the per-language migration steps, and the validation performed. It matches the file-level summaries.

Finishing Touches

Simplify code

  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

Recent review info

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 09ff23ab-7ec8-455d-a868-2db3cf9faa0c
Commits

Reviewing files that changed from the base of the PR and between ca88fc1 and 7678090.

Files ignored due to path filters: 1

  • Cargo.lock is excluded by !**/*.lock
Files selected for processing: 86

  • Cargo.toml
  • cpp/obs/src/moq-settings.cpp
  • cpp/obs/test/moq-settings-test.cpp
  • dart/moq/lib/src/aliases.dart
  • dart/moq/test/moq_test.dart
  • dart/moq_ffi/lib/src/moq.dart
  • doc/bin/relay/config.md
  • go/wrapper/client.go
  • go/wrapper/origin.go
  • go/wrapper/reconnect_test.go
  • go/wrapper/types.go
  • kt/moq/src/jvmAndAndroidMain/kotlin/dev/moq/Aliases.kt
  • kt/moq/src/jvmAndAndroidTest/kotlin/dev/moq/SmokeTest.kt
  • nix/Cargo.nix
  • py/moq-rs/README.md
  • py/moq-rs/tests/test_server.py
  • quest/m2/congestion-candidates.md
  • quest/m3/vendor-sync/scream.md
  • quest/m3/vendor-sync/webrtc.md
  • quest/m3/webrtc/README.md
  • quest/m4/quic-realtime.md
  • rs/moq-c/src/api.rs
  • rs/moq-congestion-lab/.rustfmt.toml
  • rs/moq-congestion-lab/Cargo.toml
  • rs/moq-congestion-lab/LICENSE-APACHE
  • rs/moq-congestion-lab/LICENSE-MIT
  • rs/moq-congestion-lab/README.md
  • rs/moq-congestion-lab/src/gcc/AUTHORS
  • rs/moq-congestion-lab/src/gcc/LICENSE
  • rs/moq-congestion-lab/src/gcc/PATENTS
  • rs/moq-congestion-lab/src/gcc/alr_detector.rs
  • rs/moq-congestion-lab/src/gcc/api/mod.rs
  • rs/moq-congestion-lab/src/gcc/api/transport/bandwidth_usage.rs
  • rs/moq-congestion-lab/src/gcc/api/transport/mod.rs
  • rs/moq-congestion-lab/src/gcc/api/transport/network_types.rs
  • rs/moq-congestion-lab/src/gcc/api/units/data_rate.rs
  • rs/moq-congestion-lab/src/gcc/api/units/data_size.rs
  • rs/moq-congestion-lab/src/gcc/api/units/frequency.rs
  • rs/moq-congestion-lab/src/gcc/api/units/mod.rs
  • rs/moq-congestion-lab/src/gcc/api/units/time_delta.rs
  • rs/moq-congestion-lab/src/gcc/api/units/timestamp.rs
  • rs/moq-congestion-lab/src/gcc/api/units/unit_base.rs
  • rs/moq-congestion-lab/src/gcc/bench.rs
  • rs/moq-congestion-lab/src/gcc/congestion_window_pushback_controller.rs
  • rs/moq-congestion-lab/src/gcc/delay_based_bwe.rs
  • rs/moq-congestion-lab/src/gcc/delay_increase_detector_interface.rs
  • rs/moq-congestion-lab/src/gcc/experiments/field_trials.rs
  • rs/moq-congestion-lab/src/gcc/experiments/mod.rs
  • rs/moq-congestion-lab/src/gcc/experiments/rate_control.rs
  • rs/moq-congestion-lab/src/gcc/goog_cc_network_control.rs
  • rs/moq-congestion-lab/src/gcc/inter_arrival_delta.rs
  • rs/moq-congestion-lab/src/gcc/link_capacity_estimator.rs
  • rs/moq-congestion-lab/src/gcc/loss_based_bwe_v2.rs
  • rs/moq-congestion-lab/src/gcc/mod.rs
  • rs/moq-congestion-lab/src/gcc/pacing/bitrate_prober.rs
  • rs/moq-congestion-lab/src/gcc/pacing/interval_budget.rs
  • rs/moq-congestion-lab/src/gcc/pacing/mod.rs
  • rs/moq-congestion-lab/src/gcc/probe_bitrate_estimator.rs
  • rs/moq-congestion-lab/src/gcc/probe_controller.rs
  • rs/moq-congestion-lab/src/gcc/remote_bitrate_estimator/aimd_rate_control.rs
  • rs/moq-congestion-lab/src/gcc/remote_bitrate_estimator/bwe_defines.rs
  • rs/moq-congestion-lab/src/gcc/remote_bitrate_estimator/mod.rs
  • rs/moq-congestion-lab/src/gcc/robust_throughput_estimator.rs
  • rs/moq-congestion-lab/src/gcc/send_side_bandwidth_estimation.rs
  • rs/moq-congestion-lab/src/gcc/tests.rs
  • rs/moq-congestion-lab/src/gcc/trendline_estimator.rs
  • rs/moq-congestion-lab/src/lib.rs
  • rs/moq-congestion-lab/src/scream.rs
  • rs/moq-congestion/Cargo.toml
  • rs/moq-congestion/README.md
  • rs/moq-congestion/src/bbr3/mod.rs
  • rs/moq-congestion/src/lib.rs
  • rs/moq-congestion/src/pacer.rs
  • rs/moq-congestion/src/rate.rs
  • rs/moq-ffi/src/session.rs
  • rs/moq-ffi/src/test.rs
  • rs/moq-ffi/src/test/rung.rs
  • rs/moq-quic/Cargo.toml
  • rs/moq-quic/src/tests/probe.rs
  • rs/moq-quic/src/tests/scream.rs
  • rs/moq-tokio/Cargo.toml
  • rs/moq-tokio/src/quic.rs
  • rs/moq-tokio/tests/integration/scream.rs
  • rs/workspace-hack/Cargo.toml
  • sh/gh/nightly.sh
  • swift/Sources/Moq/Aliases.swift
Files with no reviewable changes: 1

  • quest/m2/congestion-candidates.md

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

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant