fix: restore MoQServer optimized defaults and enable pacing in buildTransportSettings#148
Merged
fix: restore MoQServer optimized defaults and enable pacing in buildTransportSettings#148
Conversation
…ransportSettings Supplying a TransportSettings to MoQServer bypasses its optimized defaults (GSO batching, ContinuousMemory data path, large cwnd, etc.). Replicate those defaults explicitly before applying config overrides so nothing is silently lost. Also enables pacingEnabled unconditionally — BBR requires it (mvfst falls back to Cubic without it), and pacing is beneficial for all supported CC algorithms. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
gmarzot
approved these changes
Apr 10, 2026
Contributor
gmarzot
left a comment
gmarzot
left a comment
There was a problem hiding this comment.
@gmarzot reviewed 1 file and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on afrind).
gmarzot
added a commit
that referenced
this pull request
Apr 12, 2026
…ngs) PR #148 correctly sets Copa + pacing as the server-side default in buildTransportSettings(), but the config layer default in Config.h was still "bbr". When no congestion_control is specified in the YAML config, the config default overwrites Copa with BBR. BBR without pacing triggers mvfst's "Unpaced BBR isn't supported" fallback to Cubic. Change the config default to "copa" so it matches the transport settings baseline. Users who explicitly set congestion_control in their config are unaffected.
3 tasks
gmarzot
added a commit
that referenced
this pull request
Apr 12, 2026
- Change Config.h default from "bbr" to "copa" to match the transport settings baseline established by PR #148 (buildTransportSettings sets Copa + pacing). Without this, the config layer silently overwrites Copa with BBR, triggering mvfst's "Unpaced BBR isn't supported" fallback to Cubic. - Fix the picoquic CC algorithm allowlist in ConfigResolver.cpp: - Remove "bbr" (not registered in picoquic; the actual name is "bbr1") - Add "copa" (compiled in and functional, was incorrectly excluded) - Update ParsedConfig.h description and test assertion to match. Users who explicitly set cc_algo in their config are unaffected.
gmarzot
added a commit
that referenced
this pull request
Apr 13, 2026
picoquic registers its BBR as "bbr1", not "bbr". Users shouldn't need to know this — cc_algo: bbr should work for both stacks. Changes: - Add "bbr" and "copa" to the picoquic validator allowlist (both are valid: "bbr" is mapped, "copa" is natively registered) - After validation, transparently map "bbr" → "bbr1" for picoquic listeners before the config reaches the transport layer - Add test: PicoBbrMappedToBbr1 verifies the mapping - Update ParsedConfig.h description to list "bbr" for picoquic The default cc_algo remains "bbr" (unchanged). With PR #148's pacing fix, BBR works correctly on mvfst without warnings.
gmarzot
added a commit
that referenced
this pull request
Apr 13, 2026
picoquic registers its BBR as "bbr1", not "bbr". Users shouldn't need to know this — cc_algo: bbr should work for both stacks. Changes: - Add "bbr" and "copa" to the picoquic validator allowlist (both are valid: "bbr" is mapped, "copa" is natively registered) - After validation, transparently map "bbr" → "bbr1" for picoquic listeners before the config reaches the transport layer - Add test: PicoBbrMappedToBbr1 verifies the mapping - Update ParsedConfig.h description to list "bbr" for picoquic The default cc_algo remains "bbr" (unchanged). With PR #148's pacing fix, BBR works correctly on mvfst without warnings.
gmarzot
added a commit
that referenced
this pull request
Apr 14, 2026
…ngs) PR #148 correctly sets Copa + pacing as the server-side default in buildTransportSettings(), but the config layer default in Config.h was still "bbr". When no congestion_control is specified in the YAML config, the config default overwrites Copa with BBR. BBR without pacing triggers mvfst's "Unpaced BBR isn't supported" fallback to Cubic. Change the config default to "copa" so it matches the transport settings baseline. Users who explicitly set congestion_control in their config are unaffected.
gmarzot
added a commit
that referenced
this pull request
Apr 14, 2026
picoquic registers its BBR as "bbr1", not "bbr". Users shouldn't need to know this — cc_algo: bbr should work for both stacks. Changes: - Add "bbr" and "copa" to the picoquic validator allowlist (both are valid: "bbr" is mapped, "copa" is natively registered) - After validation, transparently map "bbr" → "bbr1" for picoquic listeners before the config reaches the transport layer - Add test: PicoBbrMappedToBbr1 verifies the mapping - Update ParsedConfig.h description to list "bbr" for picoquic The default cc_algo remains "bbr" (unchanged). With PR #148's pacing fix, BBR works correctly on mvfst without warnings.
gmarzot
added a commit
that referenced
this pull request
Apr 14, 2026
* config: default congestion control to copa (match buildTransportSettings) PR #148 correctly sets Copa + pacing as the server-side default in buildTransportSettings(), but the config layer default in Config.h was still "bbr". When no congestion_control is specified in the YAML config, the config default overwrites Copa with BBR. BBR without pacing triggers mvfst's "Unpaced BBR isn't supported" fallback to Cubic. Change the config default to "copa" so it matches the transport settings baseline. Users who explicitly set congestion_control in their config are unaffected. * config: map bbr → bbr1 for picoquic, fix CC validator allowlists picoquic registers its BBR as "bbr1", not "bbr". Users shouldn't need to know this — cc_algo: bbr should work for both stacks. Changes: - Add "bbr" and "copa" to the picoquic validator allowlist (both are valid: "bbr" is mapped, "copa" is natively registered) - After validation, transparently map "bbr" → "bbr1" for picoquic listeners before the config reaches the transport layer - Add test: PicoBbrMappedToBbr1 verifies the mapping - Update ParsedConfig.h description to list "bbr" for picoquic The default cc_algo remains "bbr" (unchanged). With PR #148's pacing fix, BBR works correctly on mvfst without warnings. * config: add bbr to picoquic allowlist, add production config template Suhas confirmed picoquic's "bbr" is BBRv3 (current) and "bbr1" is the legacy v1. The earlier bbr → bbr1 mapping was a silent downgrade; remove it. With moxygen #139 landed (registers all picoquic CC algorithms), "bbr" works natively for pico. Changes: - Validator allowlist: accept "bbr" for picoquic (no mapping) - ParsedConfig.h description updated to match - Drop test for bbr → bbr1 mapping (mapping removed) - Add docker/config.production.yaml as deployment template (dual listener: mvfst + picoquic, both cc_algo: bbr, exact paths) - Bump deps/moxygen submodule to 9e06544d (pico CC fix + latest upstream sync) copa is not in the picoquic allowlist — need to verify the picoquic build actually has it before adding. * retrigger ci
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supplying a TransportSettings to MoQServer bypasses its optimized defaults (GSO batching, ContinuousMemory data path, large cwnd, etc.). Replicate those defaults explicitly before applying config overrides so nothing is silently lost.
Also enables pacingEnabled unconditionally — BBR requires it (mvfst falls back to Cubic without it), and pacing is beneficial for all supported CC algorithms.
This change is