tcp: implement the RFC 4015 response to spurious loss recovery - #14458
Open
davidbell217 wants to merge 1 commit into
Open
tcp: implement the RFC 4015 response to spurious loss recovery#14458davidbell217 wants to merge 1 commit into
davidbell217 wants to merge 1 commit into
Conversation
RFC 3522 Eifel detection sets sender.spuriousRecovery and the SpuriousRecovery metrics, but nothing consumed the signal: leaveRecovery unconditionally deflated SndCwnd to Ssthresh, so a recovery the sender itself proved spurious still cost a permanent multiplicative decrease, and under sustained path jitter the sender ratchets its throughput down episode after episode. Capture pipe_prev (RFC 4015 Section 3 step (0)) at every loss-recovery entry point before the congestion controller reduces Ssthresh, and on exit from a recovery detected spurious restore Ssthresh to it and slow-start back from FlightSize + IW (step (9)) instead of keeping the deflation. The response is applied at the single point every recovery exits through — the Open transition in handleRcvdSegment — after the ACK-removal loop, when Outstanding is the true FlightSize, so the send it allows is bounded by IW. The capture is consumed on use, so the response applies at most once per episode. detectSpuriousRecovery now ignores ACKs carrying no Timestamps option: a TSEcr of zero previously compared as smaller than RetransmitTS and marked a genuine recovery spurious, which was harmless while the flag only fed a metric but wrong once a response consumes it. Extend the two spurious-recovery e2e tests to drive the connection out of recovery on each exit path and assert Ssthresh/SndCwnd are restored; both extensions fail without the fix. Fixes google#14102 Assisted-by: Claude Code
davidbell217
force-pushed
the
rfc4015-spurious-recovery-response
branch
from
August 27, 2026 20:56
be53c9a to
190e04d
Compare
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
1 participant
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.
Fixes #14102 (cc @nybidari).
Implements the RFC 4015 congestion control response to spurious recovery, per the notes on the issue:
pipe_previs captured at all five recovery entry points (detectLoss, RACK reorder timer, TLP loss, RTO expiry) beforeHandleLossDetected()/HandleRTOExpired()cutSsthresh— atenterRecovery()it's already too late.SndCwndstands in for FlightSize (both packet-counted;Outstandingis already decimated by the ACKs that trigger detection). A never-reducedSsthresh(InitialSsthresh) is not restored: it holds no pipe estimate, and restoring it re-created the collapse on our fixture.Ssthresh = pipe_prev(only if that raises it) andSndCwnd = FlightSize + IW. cwnd is not restored directly — a direct restore burst-collapsed 24/24 runs, as warned in the issue.leaveRecovery, pure RTO directly) ends at theOpentransition inhandleRcvdSegment, so the response applies exactly there — after the ACK-removal loop, whereOutstandingis the true FlightSize and the allowed send is bounded by IW. The capture is consumed on use, so it applies at most once per episode.sender; nothing added toTCPSenderState.TestDetectSpuriousRecoveryWithRTOandTestSACKDetectSpuriousRecoveryWithDupACKnow drive the connection out of recovery and assertSsthresh/SndCwndare restored. Both fail without the fix (Ssthresh = 2, want = 10, one per exit path).One enabling detection fix:
detectSpuriousRecoverynow ignores ACKs with no Timestamps option (!SendTSOk || TSEcr == 0). PreviouslyTSEcr = 0compared as< RetransmitTSand marked a genuine recovery spurious — harmless while only a metric consumed the flag, butTestSACKRecovery(whose crafted ACKs carry no TS option) fails once a response does.Benchmark, on the fixture from #14102 (195 ms RTT, ±15 ms jitter, 3% of packets delayed +30 ms, no loss; 24 runs/arm; #14092's fix applied in both arms; pinned = run exceeds 15 s):
Episodes still fire either way; the response stops them compounding. A clean fixture (3% reorder, no jitter) shows no regression.
Scope: RFC 4015 specifies the response for spurious timeouts; applying it to fast/SACK recovery too matches netstack's detection, which deliberately doesn't differentiate. Step (8) (
SND.NXT <- SND.MAX), steps (10)–(11) (RTT/RTO restoration), and the ECE guard (netstack has no ECN congestion response) are out of scope.