[kixelated]

Problem

The realtime uplink epic runs a bake-off between delay-based controllers fed by receive timestamps. This is the SCReAMv2 candidate (quest/m2/quic-realtime/scream.md): no Rust implementation existed.

Approach

• moq_congestion::Scream / scream::Config port the queue-delay target path of draft-ietf-ccwg-rfc8298bis-screamv2-01: a reference window that grows per acked byte (multiplicative after a quiet spell) and shrinks on loss, classic ECN-CE, and "virtual L4S" as the averaged queue delay passes half the target. The window is ref_wnd times the 1.5 to 3x overhead; pacing is 1.5x the target rate; bandwidth_estimate is the target rate. The L4S path and the competing-flow target raise are left out, per the quest.
• Queue delay is each packet's forward delay (peer receive time less send time since the controller's epoch) above the lowest seen over the last 100s (ten 10s windows, as LEDBAT keeps its base). The peer's unknown clock basis cancels. One sample per ACK: the least delayed of its packets.
• Probing: probes() is true and probe_inflight is the window while the averaged queue delay is under a quarter of the target and no reaction happened in the last RTT.
• Unit tests (mocked time) drive the controller through a modeled FIFO bottleneck: fills the path, follows a halving within a second, ignores the peer basis, tolerates 100 ppm clock drift for 5 minutes, app-limited window stops growing, probing stops on a queue, loss backs off once per reaction.
• moq-quic tests::scream (sim) runs an encoder that follows the estimate (30 fps, 2s GOP, 5x keyframes) through sim replicas of the three cellular presets. The merge gate checks two things on one seed: SCReAMv2's queue p50/p95 beat BBR3's on cellular-step, and a stopped constant-rate rung recovers only with a probe target. The nightly compare table is in just rs quic-sim.
• moq-tokio scream integration tests run the real moq-shaper presets over sockets, including near-zero as the noise floor. They are nightly, wired into just rs quic-sim.
• Harness: main's moq-dev/moq#7377 moved the sim's Bottleneck and Link into moq_quic::sim (hidden, behind cfg(test) or the sim feature). This PR adds Bottleneck::timed, Bottleneck::trace, and Link::grant there, the same pieces moq-dev/moq#7544 adds to the same file, line for line, so whichever lands second merges them cleanly.
• Main's API since: SCReAMv2 follows moq-dev/moq#7258's namespaced configs (pub mod scream with scream::Config, built through Factory, reporting Metrics, no into_any), and moq-dev/moq#7650's floor: its window is at least two datagrams of the path MTU, and it joins that floor's test in moq-congestion.

Measurements

Sim, mean of 4 seeds, adaptive media, rerun on 34f40a8300 with main's corrected pacer (moq-dev/moq#7606). Queue is per packet at the bottleneck past 3s. Utilization is goodput over capacity per phase (before, during, after the drop). Fall is when the estimate first sits at or under the dropped rate. Drain is when the queue first returns under 30ms. Rise is when the estimate reaches 80% of the restored rate.

preset	controller	queue p50/p95/p99	frame p50/p95/p99	utilization	fall	drain	rise
cellular-step	scream	21/118/196ms	68/1327/2228ms	0.82 0.85 0.83	380ms	686ms	545ms
cellular-step	scream reduce_jitter	8/42/86ms	48/948/1441ms	0.80 0.75 0.79	345ms	683ms	1268ms
cellular-step	scream target 30ms	10/57/111ms	53/1815/2106ms	0.82 0.66 0.84	340ms	779ms	552ms
cellular-step	bbr3	30/358/436ms	205/1438/1893ms	0.96 0.98 0.94	never	111ms	0ms
cellular-bursty	scream	15/109/199ms	69/241/466ms	0.84 0.81 0.84	275ms	770ms	570ms
cellular-bursty	scream reduce_jitter	7/29/51ms	59/182/432ms	0.79 0.72 0.75	230ms	386ms	2910ms
cellular-bursty	scream target 30ms	8/43/101ms	57/161/334ms	0.78 0.48 0.78	175ms	559ms	685ms
cellular-bursty	bbr3	44/140/178ms	257/857/1234ms	0.97 0.97 0.96	never	4669ms	465ms
cellular-deep	scream	16/127/325ms	77/1361/1689ms	0.82 0.77 0.81	372ms	1293ms	600ms
cellular-deep	scream reduce_jitter	12/53/105ms	63/2244/2912ms	0.81 0.65 0.79	362ms	712ms	1290ms
cellular-deep	scream target 30ms	12/84/208ms	59/1051/1432ms	0.77 0.60 0.78	335ms	995ms	818ms
cellular-deep	bbr3	51/357/494ms	241/1242/2275ms	0.97 0.98 0.96	never	5137ms	915ms

Real sockets through moq-shaper, default config, one run each on 34f40a8300. The host was busy (loadavg 17 to 20, against 1.5 to 5 for the earlier runs), which widens every number here. Queue is the forward delay above the run's minimum, so it includes the preset's jitter.

preset	queue p50/p95/p99	frame p50/p95/p99	utilization	fall	drain	rise
near-zero (noise floor)	0/0/0ms	0/1/2ms
cellular-step	32/189/280ms	70/1046/1309ms	0.82 0.85 0.95	321ms	885ms	242ms
cellular-bursty	38/126/265ms	66/291/652ms	0.81 0.86 0.90	314ms	850ms	134ms
cellular-deep	35/293/644ms	106/1858/2078ms	0.82 0.79 0.77	429ms	1446ms	230ms

Stopped rung (sim, 4 seeds): a 500 kbit/s rung on a 1 Mbit/s path that opens to 4 Mbit/s at 15s, probe target 2.5 Mbit/s.

media	probe	estimate before opening	reached target after opening	overhead	queue p95 before/after
rung with keyframes	on	146 kB/s	775ms	0.069	75/3ms
rung with keyframes	off	240 kB/s	235ms	0	30/3ms
constant rung	on	134 kB/s	880ms	0.074	38/3ms
constant rung	off	111 kB/s	never	0	12/3ms

Findings:

• SCReAMv2 holds a third of BBR3's median queue on bursty and deep (15 vs 44ms, 16 vs 51ms) and 0.7x on step (21 vs 30ms), and cuts p95 by 1.3x (bursty) to 3x (step, deep). BBR3's own queue shrank on main since the earlier table (step median 37ms then), which narrowed the step gap. SCReAMv2 gives up 11 to 21% of throughput, and its estimate drops below the new rate within about 400ms; BBR3's max filter never does in these runs.
• Frame delay: SCReAMv2's frame p50 is a third of BBR3's everywhere (68 to 77 vs 205 to 257ms). Its frame p95 is BBR3's on step and deep (1327 vs 1438ms, 1361 vs 1242ms), and a quarter of it on bursty (241 vs 857ms).
• That p95 rose with fix(congestion): preserve low-rate pacing debt and retune BBR3 #7606's pacer, not with this PR's port: rerun with the pre-fix(congestion): preserve low-rate pacing debt and retune BBR3 #7606 pacer.rs, every SCReAMv2 row reproduced the earlier table exactly (step 853ms, deep 300ms frame p95). The old pacer let keyframes (5x a delta frame) overshoot SCReAMv2's pacing of 1.5x the target rate; now they wait in the sender. The draft's media-queue terms (4.4), left out here, are what would answer that backlog. Recorded in the epic for the bake-off.
• The step-down queue tail (p99 about 200 to 330ms) is the in-flight data and the up-to-3x window meeting a 4x rate drop. reduce_jitter shrinks the window overhead under keyframe jitter and cuts the queue p95 by 2 to 4x, at another 1 to 15% of throughput, though its frame p95 is worse on deep (2244 vs 1361ms).
• Its estimate is the reference window over the RTT, not a delivery rate. With application-limited media, each keyframe's burst lets the window, and so the estimate, grow past the path: 240 kB/s on a 125 kB/s path. A rung with keyframes therefore recovers without a probe. A constant-rate rung needs the probe, and recovers under it in under a second.

Choices made without asking

Listed for review. The Plan did not settle these.

• Kept the draft's defaults (qdelay_target 60ms, REDUCE_JITTER off) and measured the variants instead. The bake-off can choose.
• One deviation from the draft, following the reference: a delay-based backoff also counts as congestion for the window's multiplicative growth. The draft's prose counts delay as congestion, but its code only stamps loss and CE. Without this, the window returned to multiplicative growth right after each backoff and oscillated.
• A second deviation, this one from both the draft's pseudocode and the reference: the increase's inflection scale scl_t is taken after the update's reduction, which is what the draft's own comment says ("ref_wnd_i is updated before this code"). A reaction makes the window it began at the new inflection, so growth right after a small backoff stays at 0.25. Taking it before, against an older inflection the window had grown past, raised follows_a_capacity_drop's post-drop queue p95 from 57.8 to 61.8ms, past the 60ms target. backoff_holds_growth_at_the_new_inflection pins this.
• CE-marked bytes still count toward the increase, which the draft leaves out. Ce carries no marked count, and adding one would be a semver-breaking field. On the classic ECN path, growth after a reaction is additive, at most one MSS per round trip, so the marked bytes are worth a fraction of an MSS against the 20% cut. The L4S path would need the count.
• One delay sample per ACK, the least delayed of its packets. With the newest packet instead, the 30ms-target variant fell to 36% utilization on cellular-bursty, reading grant bunching as queue.
• Left out the draft's optional additions: 4.5.3 undershoot guard, 4.5.2 conditional loss and policer, 4.5.4 competing-flow target, and the media-queue terms of 4.4. The guard raised utilization about 8% but doubled the p95 queue. The epic picks on queuing delay.
• No RTT fallback without receive timestamps. The epic falls back to BBR3 at the session level, and a runtime fallback inside the controller would be a silent path switch. Without timestamps, the controller sees no queue and backs off only on loss.
• MSS is the path MTU, and the minimum reference window is the draft's 3000 bytes. The window itself never drops below two datagrams of the path MTU, as fix(congestion): start and floor every window at two datagrams of the path MTU #7650 holds every controller to; at a 1200 to 1500-byte MTU that floor never binds (the minimum window is 4500 bytes).

Impact

• Additive: moq_congestion::Scream and moq_congestion::scream::Config (qdelay_target, reduce_jitter), a Factory. No wire change; nothing selects it yet. Realtime wiring is the epic's job.

Alternatives

• Joining moq-dev/moq#7544's tests::realtime harness now. That file is not on main yet (feat(congestion): BBR3 delay bound from the forward delay's trend #7544 is queued), and its bulk upload is not the workload SCReAMv2 is built for (see the comment on that PR). tests::scream drives an adaptive encoder instead. The epic's comparison step can fold both into one harness.

Follow-ups

• For the epic's bake-off: run every candidate under both workloads, bulk and an adaptive encoder, in one harness. Weigh the estimate's overshoot under app-limited bursty media for rung re-enable.
• SCReAMv2's frame tail under the corrected pacer: try the draft's media-queue terms (4.4) or a larger pacing headroom during keyframes, and measure the frame p95 against the queue it costs.
• SCReAMv2's minimum window and MSS: at 300 to 500 kbit/s and 40 to 60ms RTT, the path holds 2 to 3 packets, and 1 MSS of additive growth per RTT is a large step. The draft suggests smaller packets at low rates, which QUIC cannot easily do.

Review

• Head 34f40a8300: Codex, gpt-6.1-sol, medium effort (quest review --base origin/main, new session), round 2: no findings. Round 1 (head b75b60eb46, the merge of origin/main at 8121cf68df plus the API port): no findings. The only change between the rounds is the epic README note on the frame tail.
• Pullfrog's two port-fidelity notes on b724eb2 were both declined with evidence, documented in the code, and resolved: CE bytes in the increase and scl order. Both are listed under the choices above. CodeRabbit had no actionable comments; its docstring-coverage pre-merge warning is a generic threshold, not a finding.
• Earlier sessions: Codex gpt-6-astra round 1 on 6df823161d, no findings; round 2 on b724eb2, no findings; round 1 on 9640ad3, one finding (the base delay's history aging across a quiet spell), fixed and covered by base_delay_ages_across_a_quiet_spell.
• Checks on 34f40a8300 in nix develop: just check origin/main passed (11452 tests), quest check --base origin/main ok, the probe acceptance suite (tests::probe, ignored included, 14 passed), tests::scream (gated and compare), and moq-tokio's real-socket scream runs (4 passed). The tables above are from these runs.
• Merge base: moq-dev/moq#7544 adds the same sim.rs pieces line for line, so the merge after it lands should touch only tests/mod.rs's module list and the epic README.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

The change adds SCReAMv2 as a public congestion controller with configuration options, controller callbacks, and unit tests. It adds simulation utilities and tests for cellular uplink conditions, plus ignored real-socket QUIC measurements. Documentation and test commands now include SCReAMv2.

Merge Risk: Minimal · up to 6df82

This adds a new SCReAMv2 congestion controller that nothing selects yet, so existing behavior is unchanged. No actionable merge-blocking issue remains. The documented deviations from the draft are intentional and covered by tests.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 65.38% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title clearly identifies the main change: adding a SCReAMv2 delay-based congestion controller candidate.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description check		The pull request description directly explains the SCReAMv2 controller, its implementation, tests, simulations, integration measurements, and documented design choices. These topics match the files an…

Full details: Description check

Explanation

The pull request description directly explains the SCReAMv2 controller, its implementation, tests, simulations, integration measurements, and documented design choices. These topics match the files and objectives in the changeset.

Full details: Docstring Coverage

Explanation

Docstring coverage is 65.38% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 26 functions across 7 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

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

• X
• Mastodon
• Reddit
• LinkedIn

CodeRabbit Verify

• Run CodeRabbit Verify to test the changes in this pull request.

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: 920a231a-33d6-46a0-b502-0761b748506c

Commits

Reviewing files that changed from the base of the PR and between b724eb2 and 6df8231.

Files selected for processing: 1

• rs/moq-congestion/src/scream.rs

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

[pullfrog]

ℹ️ No critical issues — two port-fidelity notes inline.

Reviewed changes

First pullfrog pass on the initial revision: a new SCReAMv2 delay-based congestion controller and the two harnesses that measure it.

• rs/moq-congestion/src/scream.rs (new) — ports the queue-delay target path of draft-ietf-ccwg-rfc8298bis-screamv2-01: Scream/ScreamConfig, a reference window with per-acked-byte growth and loss/ECN/"virtual L4S" reduction, receive-timestamp queue delay over a 100s base, and probing.
• rs/moq-congestion/src/lib.rs — exports Scream, ScreamConfig (additive; nothing selects it yet).
• rs/moq-quic/src/tests/{mod,scream,util}.rs — sim harness pieces (Bottleneck::timed/trace, Link::grant) and tests::scream: the BBR3 comparison and probe-recovery merge-gate tests plus an #[ignore]d comparison table.
• rs/moq-tokio/tests/integration/{main,scream}.rs — nightly real-socket measurement through the moq-shaper presets.
• rs/justfile, the two READMEs, and quest/m2/quic-realtime/* — wire the nightly runs and mark the quest complete.

I verified the controller's constants and formulas against the draft (§4.2.1.4, §4.2.2) and Ericsson's ScreamV2Tx.cpp updateCwnd; the values all match the draft except the two notes below. A peer-input pass found no panic, overflow, out-of-bounds, or NaN path from untrusted receive timestamps or ACK/loss patterns.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏}

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: b724eb2

No actionable bugs found in this static review. Checked the delay baseline/quiet-spell aging, window growth and congestion reactions, pacing/probe integration, and the sim and socket measurement paths. The earlier quiet-spell finding recorded in the PR body is addressed by aging all elapsed windows and its focused test.

Direction: a separate, opt-in controller is a reasonable boundary for this bake-off, with no wire change or change to existing defaults. Reusing the pacer and controller interface keeps the production addition focused. Consolidating the duplicated measurement harness during the planned comparison remains preferable to maintaining divergent workloads. The documented bursty, application-limited estimate overshoot should remain part of that comparison rather than treating this estimate as measured delivery capacity.

Verification limits: source inspection only. I did not run tests, simulations, socket experiments, or reproduce the reported performance measurements.

(Written by OpenAI)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 6df8231, compared with b724eb2, accounting for the base merge.

No new actionable bugs found. The added backoff_holds_growth_at_the_new_inflection test meaningfully distinguishes post-reduction scaling from scaling against the older inflection: the asserted 0.25 additive factor follows from the small backoff and updated ref_wnd_i. Production behavior is unchanged; the new comments record the intentional scaling order and classic-ECN approximation discussed in the existing threads (scaling, CE accounting). The earlier quiet-spell fix remains intact.

Direction remains reasonable for the scoped delay-controller candidate. Keeping these choices explicit and pinning the behavior is preferable to expanding the public congestion interface just for the deferred L4S work.

Static inspection only; I did not run the new test or reproduce the reported queue-delay comparison.

(Written by OpenAI)

[pullfrog]

✅ No new issues found.

Reviewed changes

Incremental pass on the commits since the prior pullfrog review (b724eb2): the author's documentation of, and regression test for, the two port-fidelity notes pullfrog raised. No production behavior change.

• rs/moq-congestion/src/scream.rs — scl()'s doc now states the post-reduction evaluation order (with the draft-comment citation and the measured queue-p95 impact), and on_ce documents why CE-marked bytes still count toward the increase until the L4S path adds a count to Ce.
• rs/moq-congestion/src/scream.rs — added backoff_holds_growth_at_the_new_inflection, which pins the 0.25 additive scale after a small delay backoff and fails under the pre-reduction order.

Both prior pullfrog threads were addressed by this delta with evidence and are resolved; the new test confirms the pinned behavior.

  ^{｜ View workflow run ｜ Using deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏}

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: b75b60e, compared with 6df8231 and their respective bases.

No new actionable bugs found in the two-MTU floor, probe-hook and API adaptations. The prior quiet-spell fix remains present; the earlier CE accounting and scaling-order discussions are documented design deviations. Reusing the shared simulation helpers is sensible. Static review only; I did not run tests, simulations or benchmarks.

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 34f40a8, compared with b75b60e.

The corrected-pacer results and omitted media-queue feedback are useful context for the controller decision. No new actionable bugs found in this documentation update. I did not reproduce the measurements or run tests.

[pullfrog]

✅ No new issues found.

Reviewed changes

Incremental pass on the commits since the prior pullfrog review (837643a): two origin/main merges and the branch's adaptation to them.

• rs/moq-congestion/src/{lib,scream}.rs — adapted to main's congestion-config namespacing (ScreamConfig → scream::Config, ControllerFactory → Factory, ControllerMetrics → Metrics, dropped into_any) and added Scream to the window_holds_two_datagrams_at_large_mtu factory sweep.
• rs/moq-congestion/src/scream.rs — window() and the pacer's initial window now floor at two datagrams of the path MTU, matching main's #7650 floor for the other controllers.
• rs/moq-quic/src/tests/scream.rs, rs/moq-tokio/tests/integration/scream.rs — renamed to the namespaced API; the recorder forwards the new paces_probe / on_probe_target trait methods.
• READMEs, rs/justfile, quest/m2/quic-realtime/ — SCReAMv2 added to the controller lists and marked landed; the nightly commands are unchanged.

I confirmed against the merged main that the rename is complete (no stale ScreamConfig / ControllerFactory / ControllerMetrics left in code) and that cargo test -p moq-congestion --lib passes (365 tests, including window_holds_two_datagrams_at_large_mtu). The new floor only binds above ~2250-byte MTU, so it does not move the 1200-byte-MTU merge-gate measurements. Both prior pullfrog threads are resolved.

ℹ️ The PR description's Impact line still names the pre-merge API

After the #7258 merge the added API is moq_congestion::Scream and moq_congestion::scream::Config behind a Factory, not moq_congestion::ScreamConfig / ControllerFactory. The Impact section is what the release notes and migration guidance are drawn from, so it is worth restating with the namespaced names.

Technical details

# Stale public API names in the PR description

## Affected sites
- PR #7547 body, "Impact" section — names `moq_congestion::ScreamConfig` and `ControllerFactory`

## Required outcome
- The Impact bullet names `moq_congestion::Scream` and `moq_congestion::scream::Config`, with the `Factory` trait, matching the code after the `#7258` merge.

  ^{｜ View workflow run ｜ Using deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏}