cloudflare / cloudflare/quiche

BBRv2 connection can stay in PROBE_RTT indefinitely: exit condition is missing the minimum-window clause from the reference implementation

Open
#2,698 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
11.8k
Forks
1.1k
Avg merge
21h 9m
Merged PRs (30d)
6

Description

## Problem Description

In gcongestion BBRv2, the only way to leave PROBE_RTT is to first drain `bytes_in_flight` below `inflight_target` (0.5 × BDP), which arms a 200ms exit timer. However, the congestion window a sender can actually drain down to is floored at the *initial* congestion window, and that floor is applied **after** the PROBE_RTT mode cap. When `0.5 × BDP < initial_cwnd`, a continuously saturated sender keeps `bytes_in_flight` pinned near `initial_cwnd`, the drain condition can never be satisfied, the exit timer is never armed, and the connection remains in PROBE_RTT indefinitely with cwnd pinned at `initial_cwnd`. There is no wall-clock fallback.

Google quiche (the reference this implementation is ported from) has a second clause in the exit check that makes this state unreachable; the port dropped it.

Found by code inspection while comparing the port against the reference implementation; not (yet) tied to a production incident. It is adjacent to, but distinct from, #2496 — that report is about never *entering* PROBE_RTT; this one is about never *leaving* it.

## Reference implementation behavior

Google quiche arms the exit timer when in-flight drops below the inflight target **or** below the minimum congestion window ([`bbr2_probe_rtt.cc`](https://github.com/google/quiche/blob/main/quiche/quic/core/congestion_control/bbr2_probe_rtt.cc#L28-L41)):

```cpp
if (exit_time_ == QuicTime::Zero()) {
if (congestion_event.bytes_in_flight <= InflightTarget() ||
congestion_event.bytes_in_flight <=
sender_->GetMinimumCongestionWindow()) {
exit_time_ = congestion_event.event_time + Params().probe_rtt_duration;
```

`GetMinimumCongestionWindow()` is `cwnd_limits().Min()` — the same lower bound the sender's cwnd is clamped by. That makes the check exhaustive over both clamp outcomes: either the PROBE_RTT cap wins (`cwnd ≤ target`, so in-flight reaches the first clause) or the floor wins (`cwnd == min window`, so in-flight reaches the second clause). Either way the timer is armed within roughly one RTT, so PROBE_RTT cannot wedge.

## Relevant Source Code (0.29.3)

The port only kept the first clause ([`bbr2/probe_rtt.rs:96-105`](https://github.com/cloudflare/quiche/blob/0.29.3/quiche/src/recovery/gcongestion/bbr2/probe_rtt.rs#L96-L105)):

```rust
match self.exit_time {
None => {
if congestion_event.bytes_in_flight <=
self.inflight_target(params)
{
self.exit_time = Some(
congestion_event.event_time + params.probe_rtt_duration,
)
}
```

`inflight_target` is bare `0.5 × BDP(max_bandwidth)` with no floor ([`probe_rtt.rs:71-77`](https://github.com/cloudflare/quiche/blob/0.29.3/quiche/src/recovery/gcongestion/bbr2/probe_rtt.rs#L71-L77)).

Meanwhile the cwnd update applies the PROBE_RTT cap first, then the global limits ([`bbr2.rs:603-621`](https://github.com/cloudflare/quiche/blob/0.29.3/quiche/src/recovery/gcongestion/bbr2.rs#L603-L621)):

```rust
self.cwnd = self
.mode
.get_cwnd_limits(&self.params) // no_greater_than(min(…, inflight_target))
.apply_limits(self.cwnd);
self.cwnd = self.cwnd_limits.apply_limits(self.cwnd); // .max(lo).min(hi)
```

with `cwnd_limits.lo` set to the initial congestion window ([`bbr2.rs:517-520`](https://github.com/cloudflare/quiche/blob/0.29.3/quiche/src/recovery/gcongestion/bbr2.rs#L517-L520)). So the effective window in PROBE_RTT is `max(min(cwnd, 0.5 × BDP), initial_cwnd)`; when the target is below the floor, the cap is overridden entirely.

Since `probe_rtt_pacing_gain` and `probe_rtt_cwnd_gain` are both 1.0 ([`bbr2.rs:311-313`](https://github.com/cloudflare/quiche/blob/0.29.3/quiche/src/recovery/gcongestion/bbr2.rs#L311-L313)), the cwnd cap is the *only* drain mechanism PROBE_RTT has — and in this parameter region it is exactly the part that gets overridden.

## Failure scenario

- `initial_congestion_window` = 100 packets ≈ 135 KB (MSS 1350).
- Path: `max_bandwidth` ≈ 1 Gbps, `min_rtt` ≈ 1.6 ms → BDP ≈ 200 KB, `inflight_target` = 100 KB < 135 KB.
- A bulk transfer keeps the pipe saturated: in-flight hovers at cwnd = 135 KB, dipping by the size of each ACK aggregate when a congestion event is processed.
- Arming the exit timer requires a single event to observe in-flight ≤ 100 KB, i.e. > 35 KB acknowledged at once. Depending on ACK aggregation this may occasionally happen — or never. Until it does, the connection sits in PROBE_RTT at `initial_cwnd`, and only an application-limited dip or full quiescence (`on_exit_quiescence`) can rescue it.

Large initial windows on low-RTT paths make this region easy to reach; the trigger probability is timing-dependent, which also makes it hard to reproduce deterministically — the stuck state is directly visible in qlog as `bbr_probe_rtt` persisting far beyond `probe_rtt_duration`.

A secondary divergence from the reference widens the affected region: Google's global floor is the constant `kDefaultMinimumCongestionWindow = 4 × MSS` ([`bbr2_misc.h`](https://github.com/google/quiche/blob/main/quiche/quic/core/congestion_control/bbr2_misc.h#L69-L73), [`bbr2_sender.cc:66`](https://github.com/google/quiche/blob/main/quiche/quic/core/congestion_control/bbr2_sender.cc#L66)), whereas the port floors at the initial window. With the missing clause, Google's floor would confine the problem to BDP < ~11 KB (practically unreachable); flooring at a large initial window turns it into a reachable configuration.

## Suggested fix

Restore the reference's second clause:

```rust
if congestion_event.bytes_in_flight <= self.inflight_target(params) ||
congestion_event.bytes_in_flight <= cwnd_lower_limit
{
```

where `cwnd_lower_limit` is the sender's `cwnd_limits.min()` (needs to be plumbed into `ProbeRTT::on_congestion_event`, e.g. alongside the existing `cwnd` argument). Under the current floor semantics this makes PROBE_RTT degrade to a plain 200ms dwell when the target is below the floor — the min_rtt sample may read high, but liveness is preserved, matching the reference behavior.

Whether the floor itself should follow Google (`4 × MSS` instead of the initial window) is a separate question with much broader behavioral impact; this report only proposes restoring the exit-condition clause.

Contributor guide

Open the contributing guide

Research direction

Start in quiche/src/recovery/gcongestion/bbr2/probe_rtt.rs:96-105, then trace the cwnd limit handling in bbr2.rs:517-520 and 603-621. Follow how on_congestion_event receives the cwnd-related values and compare the exit check with the linked quiche reference. Done means the minimum-window case arms the existing probe RTT exit timer and prevents PROBE_RTT from remaining indefinitely.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
networking
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.