Skip to content

recovery: fix loss time selection across packet number spaces - #2701

Open
PranshulSoni wants to merge 1 commit into
cloudflare:masterfrom
PranshulSoni:fix/loss-time-earliest-deadline
Open

recovery: fix loss time selection across packet number spaces#2701
PranshulSoni wants to merge 1 commit into
cloudflare:masterfrom
PranshulSoni:fix/loss-time-earliest-deadline

Conversation

@PranshulSoni

Copy link
Copy Markdown

Fixes #2694

Summary

loss_time_and_space() in both recovery implementations compared Option<Instant> values directly:

if time.is_none() || new_time < time {

In Rust, None < Some(_) is true, so a packet number space with no loss time could overwrite an earlier space's valid deadline. The RFC 9002 Section 6.2.1 GetLossTimeAndSpace() pseudocode guards this with time == 0 checks: an absent deadline (0) must never replace an existing one.

Concretely, with Initial = Some(t+30ms), Handshake = Some(t+10ms), Application = None, the helper returned (None, Application) instead of (Some(t+10ms), Handshake). Both set_loss_detection_timer() and on_loss_detection_timeout() then missed the time-threshold loss-detection branch: the timer fell through to the PTO path and pending time-threshold loss detection was delayed or skipped.

Solution

Skip spaces without a loss time during the scan instead of comparing the options directly:

if new_time.is_some() && (time.is_none() || new_time < time) {

Applied identically to quiche/src/recovery/congestion/recovery.rs and quiche/src/recovery/gcongestion/recovery.rs, which share the same helper logic but intentionally keep parallel implementations.

Testing

  • Added loss_time_and_space_ignores_empty_spaces to both implementations' test modules, reproducing the exact scenario from the issue. Verified it fails against the pre-fix code (left: None, matching the reported behavior) and passes after the fix.
  • cargo test -p quiche --lib: 1091 passed
  • cargo test --workspace --all-targets --features=async,ffi,qlog: all passed (including BBR2-gcongestion parameterized tests)
  • cargo test --doc --features=async,ffi,qlog --workspace: all passed
  • cargo +nightly fmt -- --check: clean
  • cargo clippy --features=async,ffi,qlog --workspace -- -D warnings: clean

loss_time_and_space() compared Option<Instant> values directly, and in
Rust None sorts before Some(_), so a packet number space without a loss
time could overwrite an earlier space's valid deadline. When that
happened, set_loss_detection_timer() missed the time-threshold loss
detection deadline and on_loss_detection_timeout() fell through to the
PTO path, delaying or skipping time-threshold loss detection.

Ignore spaces without a loss time when selecting the earliest deadline,
matching the GetLossTimeAndSpace() pseudocode in RFC 9002 Section 6.2.1,
in both the legacy and gcongestion recovery implementations.

Regression test fails before the fix and passes after.
@PranshulSoni
PranshulSoni requested a review from a team as a code owner September 3, 2026 20:44
Copilot AI lite review requested due to automatic review settings September 3, 2026 20:44

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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.

Loss-time selection can discard an earlier deadline

2 participants