-
-
Notifications
You must be signed in to change notification settings - Fork 112
fix(ledbat): prevent ssthresh death spiral on high-BDP paths #2579
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The RTO handler was using `max(cwnd/2, 2*min_cwnd)` as the ssthresh floor, which is only ~5KB. This caused a death spiral on high-latency paths where: 1. Timeout occurs before cwnd can grow 2. ssthresh = max(small_cwnd/2, 5KB) = 5KB 3. Slow start exits almost immediately 4. Repeat - ssthresh stays at 5KB forever Now uses `initial_ssthresh` (typically 1MB) as the floor, ensuring slow start can still achieve reasonable throughput after recovery. Adds regression test that verifies repeated timeouts don't collapse ssthresh below a usable floor for high-BDP paths. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
|
LEDBAT++ Compliance Concern Using initial_ssthresh (1MB) as floor means after timeout: This is more aggressive than the spec intends. The spec’s small floor (5KB) ensures conservative recovery after severe congestion. Potential RiskOn low-BDP paths (short RTT, limited bandwidth), a 1MB ssthresh floor could cause: |
iduartgomez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more conservative fix might use max(cwnd/2, min(initial_ssthresh, estimated_BDP)), but that adds complexity.
|
Thanks for the review and the better solution, Nacho. You're right that using Your approach in #2580 and #2582 is superior:
Closing this PR since the issue is now properly fixed by your merged PRs. [AI-assisted - Claude] |
|
Update: Per Nacho's guidance on Matrix:
He also noted there are throughput benchmarks (not run automatically in CI due to stability/time) that could be tuned/expanded to reproduce these high-BDP path scenarios. Worth considering for future regression prevention. [AI-assisted - Claude] |
Problem
Despite multiple LEDBAT fixes in v0.1.78 (#2574, #2549, #2532, #2510, #2467, #2472), large contract transfers from nova to technic still take 18+ seconds instead of the expected 2-7 seconds. The logs show ssthresh stuck at 5KB when it should be 1MB.
Root cause: The RTO (retransmission timeout) handler at
ledbat.rs:1403calculates:With
min_cwnd = 2_848 bytes(2 × MSS), the floor is only 5,696 bytes (~5.6KB).Death spiral sequence:
This matches the "new_ssthresh_kb=5" seen in production logs.
Why CI Didn't Catch This
Existing timeout tests verified that ssthresh = old_cwnd/2, but didn't test the floor behavior. The floor of
2*min_cwndwas fine for low-RTT paths but catastrophic for high-BDP paths (135ms+ RTT) where transfers need ssthresh in the MB range to achieve reasonable throughput.The new test
test_repeated_timeouts_maintain_usable_ssthresh_flooris general: it simulates a high-BDP path and verifies that after repeated timeouts, the controller can still achieve reasonable throughput (≥5 Mbit/s). This catches any future regression that collapses ssthresh to a tiny value.Approach
Store the initial ssthresh value in a new
initial_ssthreshfield and use it as the floor inon_timeout():This ensures that even after repeated timeouts, slow start can still ramp up to the configured ssthresh (typically 1MB), enabling reasonable throughput on high-BDP paths.
Testing
test_repeated_timeouts_maintain_usable_ssthresh_floormax(cwnd/2, initial_ssthresh)behaviorFixes
Closes #2578
[AI-assisted - Claude]