Skip to content

Commit 0441d08

Browse files
committed
Let an idle CID update past a stale ping-pong guard
Misc.IdleDestCidChange failed both kernel BVTs, and a bisect settled where it came from: 1cee9be, the per-path MTU probe, fails 2 of 4 kernel jobs while its parent 2674a37 passes 4 of 4. Three other control refs pass 12 of 12 between them. It never reproduces on Linux. What the ETL trace shows. RetirePriorTo is not a decision, it is arithmetic -- Sequence + 1 - SourceCidLimit, with the limit at 4 -- so once either side advertises a source CID at sequence 4 or above the peer is forced to replace its destination CIDs. That runs QuicPathIDReplaceRetiredCids, which raises Path->InitiatedCidUpdate and, unlike QuicPathIDRetireCurrentDestCid, does not count the update. The flag is cleared in exactly one place, on a peer CID change, which need never come. The connection then goes quiet, and the idle update in QuicSendFlush is refused by !Path->InitiatedCidUpdate for the rest of the connection's life. The flag stops a change of ours and the peer's answer to it from ping-ponging, so it is wanted while an answer might still arrive. Nothing has been sent or received for the whole idle interval by the time this check runs, so anything pending is long over and the flag is stale. It is cleared here rather than consulted. Behaviour during an active exchange is unchanged. Not the earlier theory. I first blamed Send->LastFlushTime being stamped by a probe that gave up, and fixed that in 0419a77. That fix is right on its own terms -- a probe that sends nothing should not reset the idle clock -- but it was not this, and it did not help. Local: IdleDestCidChange 20/20, targeted 40/40, 172 in the path sweep and 1443 in the broad sweep, clang-tidy 21 clean. Since this does not reproduce on Linux, the real check is the kernel BVT, twice -- one 4/4 run leaves about a 6% chance of missing a 50% failure.
1 parent 0419a77 commit 0441d08

1 file changed

Lines changed: 19 additions & 2 deletions

File tree

src/core/send.c

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1966,8 +1966,25 @@ QuicSendFlush(
19661966
//
19671967
if (Connection->Settings.DestCidUpdateIdleTimeoutMs != 0 &&
19681968
Send->LastFlushTimeValid &&
1969-
CxPlatTimeDiff64(Send->LastFlushTime, TimeNow) >= MS_TO_US(Connection->Settings.DestCidUpdateIdleTimeoutMs) &&
1970-
!Path->InitiatedCidUpdate) {
1969+
CxPlatTimeDiff64(Send->LastFlushTime, TimeNow) >= MS_TO_US(Connection->Settings.DestCidUpdateIdleTimeoutMs)) {
1970+
//
1971+
// InitiatedCidUpdate is not consulted here, and is cleared instead.
1972+
//
1973+
// It exists to stop a change of ours and the peer's answer to it from
1974+
// ping-ponging: while it is set, a peer CID change clears it rather
1975+
// than provoking another change from us. That only needs to hold for
1976+
// as long as an answer might still be coming, and nothing has been
1977+
// sent or received on this path for the whole idle interval, so
1978+
// anything still pending is long over.
1979+
//
1980+
// Leaving it set here is what broke Misc.IdleDestCidChange. It is also
1981+
// raised by QuicPathIDReplaceRetiredCids, for a replacement the peer
1982+
// forced on us with retire_prior_to -- and that one is only ever
1983+
// cleared by a peer CID change, which need never come. A connection
1984+
// that took a forced replacement and then went quiet could not do an
1985+
// idle update again for the rest of its life.
1986+
//
1987+
Path->InitiatedCidUpdate = FALSE;
19711988
if (QuicConnRetireCurrentDestCid(Connection, Path)) {
19721989
Path->InitiatedCidUpdate = TRUE;
19731990
}

0 commit comments

Comments
 (0)