fix(quic): rotate DCID off failed CID on path validation timeout
Pre-fix, PathValidator.checkValidationTimeout queued a
RETIRE_CONNECTION_ID for the failed sequence number while leaving
QuicConnection.destinationConnectionId stamping that same CID. The
strict servers (quic-go / picoquic / msquic / mvfst) processed the
RETIRE, dropped their routing entry, then closed us with
PROTOCOL_VIOLATION the next time a packet arrived stamped with
the just-retired CID — visible in qlog as two RETIRE_CONNECTION_ID
frames within ~500 ms followed by "retired connection ID N, which
was used as the Destination Connection ID on this packet".
Reproducer: setting proactiveDcidRotationMillis=4_000L on
QuicConnection drove the trigger every 4 s; rebind-port vs quic-go
then failed at ~10 s with the close above. Reverted in this commit;
the experiment fields are gone.
The fix splits the timeout outcome into three cases:
- NotTimedOut — budget hasn't elapsed.
- RecoveredOnSpare — rotated active to the lowest spare CID;
queued RETIRE for the failed seq; the
QuicConnection wrapper synchronously
updates destinationConnectionId so the
next outbound stamps the new CID, atomic
under streamsLock with the validator
state change.
- StuckOnFailedCid — no spare available; KEEP the failed seq
active and DO NOT queue retire. The
writer keeps stamping the unvalidated
CID; if the path is genuinely dead the
connection idles out, and once a
NEW_CONNECTION_ID arrives the next
trigger rotates cleanly.
After the fix, rebind-port vs quic-go fails at the 60 s test
timeout with server-side trigger=idle_timeout (the Task 2 issue —
strict servers gate on fresh DCID at new src port, which we can't
synchronize with the sim's rebind cadence) instead of the
PROTOCOL_VIOLATION close.
Tests:
- PathValidatorTest:
validationTimeoutAfter3PtoTransitionsToFailedAndRetiresFailedCidWithSpare
timeoutWithoutSpareKeepsFailedSeqActiveAndDoesNotRetire
twoConsecutiveFailedValidationsRetireAllAbandonedSequencesWithSpare
- ClientPathMigrationTest:
backToBackSuccessfulMigrationsRetireExactlyOnePerRotationAndStampNewDcid
validationTimeoutWithSpareRotatesDcidAndRetiresFailedSeq
validationTimeoutWithoutSpareKeepsActiveCidAndDoesNotRetire
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Vitor Pamplona
@@ -589,28 +589,125 @@ class PathValidator(
|
||||
/**
|
||||
* RFC 9000 §8.2.4: abandon validation if more than 3 * PTO has
|
||||
* elapsed since the challenge went out without a matching
|
||||
* response. Returns the abandoned context (so the caller can
|
||||
* surface a qlog event / decide whether to attempt another
|
||||
* rotation), or null if no validation is in progress / the
|
||||
* timer hasn't fired yet.
|
||||
* response.
|
||||
*
|
||||
* On abandonment the new CID's sequence is queued for
|
||||
* RETIRE_CONNECTION_ID. We probed with that CID — even though
|
||||
* the peer never echoed our challenge, our challenge packet
|
||||
* may have reached them and they've stamped routing state
|
||||
* against the CID. Retiring it tells them they can drop that
|
||||
* state. RFC 9000 §5.1.2: a connection ID that was used and
|
||||
* then abandoned MUST be retired.
|
||||
* On abandonment we probed with the new CID — even though the
|
||||
* peer never echoed our challenge, our challenge packet may
|
||||
* have reached them and they've stamped routing state against
|
||||
* the CID. So we'd like to retire it (RFC 9000 §5.1.2: a
|
||||
* connection ID that was used and then abandoned MUST be
|
||||
* retired). BUT — under the abrupt-migration model, the
|
||||
* writer's outbound DCID is currently STAMPING the failed
|
||||
* CID. If we queue a RETIRE_CONNECTION_ID for it without
|
||||
* rotating the writer to a different CID, the next outbound
|
||||
* packet carries both the RETIRE frame AND the now-retired
|
||||
* CID as its destination — quic-go / picoquic / msquic /
|
||||
* mvfst correctly read that as PROTOCOL_VIOLATION (see
|
||||
* `quic/plans/2026-05-08-rebind-port-bug.md`).
|
||||
*
|
||||
* So on timeout we have two cases:
|
||||
*
|
||||
* - **Spare available** ([TimeoutOutcome.RecoveredOnSpare]):
|
||||
* pick the lowest-sequence unused CID, rotate
|
||||
* [activeCidSequence] to it, and queue
|
||||
* `RETIRE_CONNECTION_ID` for the failed sequence. The
|
||||
* caller MUST swap the connection's
|
||||
* `destinationConnectionId` to
|
||||
* [RecoveredOnSpare.newConnectionId] atomically with the
|
||||
* state change so the next outbound stamps the rotated
|
||||
* CID, not the retired one.
|
||||
*
|
||||
* - **No spare** ([TimeoutOutcome.StuckOnFailedCid]): keep
|
||||
* [activeCidSequence] on the failed sequence and do NOT
|
||||
* queue retire. The validation didn't succeed but the CID
|
||||
* itself is still "ours" until we explicitly retire it —
|
||||
* the peer hasn't been told to drop it. The writer keeps
|
||||
* using it; if the path actually still works, traffic
|
||||
* resumes; if it really is dead, the next idle-timeout or
|
||||
* application-layer close handles the connection. Once a
|
||||
* spare arrives via NEW_CONNECTION_ID we'll rotate on the
|
||||
* next trigger.
|
||||
*
|
||||
* Returns [TimeoutOutcome.NotTimedOut] if no validation is in
|
||||
* progress or the budget hasn't elapsed yet.
|
||||
*/
|
||||
fun checkValidationTimeout(nowMillis: Long): PathValidationState.Validating? {
|
||||
val current = state as? PathValidationState.Validating ?: return null
|
||||
fun checkValidationTimeout(nowMillis: Long): TimeoutOutcome {
|
||||
val current = state as? PathValidationState.Validating ?: return TimeoutOutcome.NotTimedOut
|
||||
val elapsed = nowMillis - current.startedAtMillis
|
||||
val budget = (current.priorPtoMillis * VALIDATION_PTO_BUDGET_MULTIPLIER).coerceAtLeast(MIN_VALIDATION_BUDGET_MS)
|
||||
if (elapsed < budget) return null
|
||||
queueRetireSequence(current.newCidSequence)
|
||||
if (elapsed < budget) return TimeoutOutcome.NotTimedOut
|
||||
// Drop any PATH_CHALLENGE we hadn't yet drained for this
|
||||
// attempt — the writer mustn't emit them onto an abandoned
|
||||
// validation cycle.
|
||||
pendingChallenges.removeAll { it.contentEquals(current.challengeData) }
|
||||
state = PathValidationState.Failed
|
||||
failedValidations += 1
|
||||
return current
|
||||
val sparePair = unusedCids.entries.firstOrNull()
|
||||
if (sparePair == null) {
|
||||
// No spare to rotate into. KEEP the failed seq active —
|
||||
// queuing a retire here would have us stamping a retired
|
||||
// CID on every subsequent packet (the very bug we're
|
||||
// guarding against). The CID is still valid from the
|
||||
// peer's perspective until we explicitly retire it.
|
||||
return TimeoutOutcome.StuckOnFailedCid(abandoned = current)
|
||||
}
|
||||
val (spareSeq, spareEntry) = sparePair
|
||||
unusedCids.remove(spareSeq)
|
||||
// Atomic with the rotation: queue retire AND bump active to
|
||||
// the spare. The caller (connection wrapper) MUST also
|
||||
// update its `destinationConnectionId` to the new bytes
|
||||
// before releasing the lock.
|
||||
queueRetireSequence(current.newCidSequence)
|
||||
activeCidSequence = spareSeq
|
||||
return TimeoutOutcome.RecoveredOnSpare(
|
||||
abandoned = current,
|
||||
newSequence = spareSeq,
|
||||
newConnectionId = spareEntry.connectionId,
|
||||
)
|
||||
}
|
||||
|
||||
sealed class TimeoutOutcome {
|
||||
/** No validation in progress, or budget hasn't elapsed yet. */
|
||||
object NotTimedOut : TimeoutOutcome()
|
||||
|
||||
/**
|
||||
* Validation timed out and we rotated [activeCidSequence]
|
||||
* to a fresh spare. The caller MUST update the
|
||||
* connection's `destinationConnectionId` to
|
||||
* [newConnectionId] atomically with this transition.
|
||||
* `RETIRE_CONNECTION_ID` for the abandoned seq has been
|
||||
* queued for the next outbound.
|
||||
*/
|
||||
data class RecoveredOnSpare(
|
||||
val abandoned: PathValidationState.Validating,
|
||||
val newSequence: Long,
|
||||
val newConnectionId: ByteArray,
|
||||
) : TimeoutOutcome() {
|
||||
override fun equals(other: Any?): Boolean {
|
||||
if (this === other) return true
|
||||
if (other !is RecoveredOnSpare) return false
|
||||
return abandoned == other.abandoned &&
|
||||
newSequence == other.newSequence &&
|
||||
newConnectionId.contentEquals(other.newConnectionId)
|
||||
}
|
||||
|
||||
override fun hashCode(): Int {
|
||||
var h = abandoned.hashCode()
|
||||
h = 31 * h + newSequence.hashCode()
|
||||
h = 31 * h + newConnectionId.contentHashCode()
|
||||
return h
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Validation timed out but no spare CID is available. The
|
||||
* failed sequence is KEPT as active (no retire queued) so
|
||||
* the writer doesn't stamp a CID it has already retired —
|
||||
* see [checkValidationTimeout] kdoc for the rationale.
|
||||
*/
|
||||
data class StuckOnFailedCid(
|
||||
val abandoned: PathValidationState.Validating,
|
||||
) : TimeoutOutcome()
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -2676,25 +2676,57 @@ class QuicConnection(
|
||||
/**
|
||||
* Check whether an outstanding PATH_CHALLENGE has exceeded the
|
||||
* 3 * PTO budget (RFC 9000 §8.2.4). Called from the driver's
|
||||
* PTO timer path. Returns true when validation just timed out.
|
||||
* PTO timer path AND from [drainOutbound]. Returns true when
|
||||
* validation just timed out.
|
||||
*
|
||||
* On timeout the validator queues the failed CID's sequence
|
||||
* number into [PathValidator.pendingRetireSequences] (Bug-2
|
||||
* fix) so the writer's next drain emits a
|
||||
* RETIRE_CONNECTION_ID. Without this the peer keeps the
|
||||
* routing entry forever — we promised by sending the challenge
|
||||
* that we might use the CID, and §5.1.2 obligates us to
|
||||
* retire it once we abandon it.
|
||||
* Two outcomes (see [PathValidator.checkValidationTimeout]):
|
||||
*
|
||||
* - [PathValidator.TimeoutOutcome.RecoveredOnSpare] — the
|
||||
* validator picked a fresh spare and rotated
|
||||
* `activeCidSequence` onto it; we MUST swap
|
||||
* [destinationConnectionId] to the new bytes synchronously
|
||||
* here, before the lock is released. Otherwise the next
|
||||
* drain would stamp the just-retired CID on the wire (the
|
||||
* bug from `quic/plans/2026-05-08-rebind-port-bug.md` —
|
||||
* quic-go / picoquic / msquic / mvfst correctly close us
|
||||
* with PROTOCOL_VIOLATION when they see "RETIRE(seq=N) +
|
||||
* DCID=seq=N bytes" in the same packet).
|
||||
* - [PathValidator.TimeoutOutcome.StuckOnFailedCid] — no
|
||||
* spare available, validator kept the failed seq active
|
||||
* and DID NOT queue retire. We stay on that CID; if the
|
||||
* path is genuinely dead the connection idles out, and if
|
||||
* a spare arrives later the next trigger will rotate
|
||||
* cleanly.
|
||||
*
|
||||
* Caller must hold [streamsLock].
|
||||
*/
|
||||
internal fun checkPathValidationTimeoutLocked(nowMillis: Long): Boolean {
|
||||
val abandoned = pathValidator.checkValidationTimeout(nowMillis) ?: return false
|
||||
qlogObserver.onPathValidationFailed(abandoned.newCidSequence)
|
||||
qlogObserver.onConnectionIdRetired("peer", abandoned.newCidSequence)
|
||||
pathValidator.acknowledgeTerminal()
|
||||
return true
|
||||
}
|
||||
internal fun checkPathValidationTimeoutLocked(nowMillis: Long): Boolean =
|
||||
when (val outcome = pathValidator.checkValidationTimeout(nowMillis)) {
|
||||
PathValidator.TimeoutOutcome.NotTimedOut -> {
|
||||
false
|
||||
}
|
||||
|
||||
is PathValidator.TimeoutOutcome.StuckOnFailedCid -> {
|
||||
qlogObserver.onPathValidationFailed(outcome.abandoned.newCidSequence)
|
||||
pathValidator.acknowledgeTerminal()
|
||||
true
|
||||
}
|
||||
|
||||
is PathValidator.TimeoutOutcome.RecoveredOnSpare -> {
|
||||
qlogObserver.onPathValidationFailed(outcome.abandoned.newCidSequence)
|
||||
qlogObserver.onConnectionIdRetired("peer", outcome.abandoned.newCidSequence)
|
||||
// Atomic with the validator's queued RETIRE: rotate
|
||||
// the writer off the abandoned CID before any drain
|
||||
// can run. Both this method and the writer drain
|
||||
// hold streamsLock, so no packet builds between
|
||||
// here and the next outbound — the swap is
|
||||
// synchronous from the wire's perspective.
|
||||
destinationConnectionId = ConnectionId(outcome.newConnectionId)
|
||||
qlogObserver.onConnectionIdActivated("peer", outcome.newSequence, outcome.newConnectionId)
|
||||
pathValidator.acknowledgeTerminal()
|
||||
true
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Queue a PATH_RESPONSE for the given [challengeData]. Called by
|
||||
|
||||
+216
@@ -27,9 +27,11 @@ import com.vitorpamplona.quic.frame.RetireConnectionIdFrame
|
||||
import com.vitorpamplona.quic.frame.decodeFrames
|
||||
import com.vitorpamplona.quic.frame.encodeFrames
|
||||
import kotlinx.coroutines.runBlocking
|
||||
import kotlinx.coroutines.sync.withLock
|
||||
import kotlin.test.Test
|
||||
import kotlin.test.assertContentEquals
|
||||
import kotlin.test.assertEquals
|
||||
import kotlin.test.assertNotEquals
|
||||
import kotlin.test.assertTrue
|
||||
|
||||
/**
|
||||
@@ -384,6 +386,220 @@ class ClientPathMigrationTest {
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun backToBackSuccessfulMigrationsRetireExactlyOnePerRotationAndStampNewDcid() =
|
||||
runBlocking {
|
||||
// Rebind-port regression: prior to the 2026-05-08 fix, two
|
||||
// back-to-back rotations could leak a packet stamped with
|
||||
// a just-retired DCID — quic-go / picoquic / msquic /
|
||||
// mvfst correctly close that as PROTOCOL_VIOLATION. This
|
||||
// test drives the validator through TWO completed
|
||||
// rotations and asserts:
|
||||
// 1. exactly ONE RETIRE_CONNECTION_ID frame is queued
|
||||
// per rotation (the prior active seq);
|
||||
// 2. EVERY post-rotation outbound packet is stamped
|
||||
// with the new DCID (no stray packet leaks the
|
||||
// retired seq).
|
||||
val (client, pipe) = newConnectedClient()
|
||||
|
||||
val cidSeq1 = ByteArray(8) { (0xA0 or it).toByte() }
|
||||
val cidSeq2 = ByteArray(8) { (0xB0 or it).toByte() }
|
||||
val token1 = ByteArray(16) { 0x71 }
|
||||
val token2 = ByteArray(16) { 0x72 }
|
||||
|
||||
// Server offers two spare CIDs up front so we can do two
|
||||
// rotations without waiting for replenishment.
|
||||
feedDatagram(
|
||||
client,
|
||||
pipe.buildServerApplicationDatagram(
|
||||
listOf(
|
||||
NewConnectionIdFrame(1L, 0L, cidSeq1, token1),
|
||||
NewConnectionIdFrame(2L, 0L, cidSeq2, token2),
|
||||
),
|
||||
)!!,
|
||||
nowMillis = 0L,
|
||||
)
|
||||
assertEquals(2, client.pathValidator.unusedCount())
|
||||
// Drain the ACK so subsequent assertions see only post-rotation traffic.
|
||||
drainOutbound(client, nowMillis = 0L)
|
||||
|
||||
// ── Rotation #1 ────────────────────────────────────────
|
||||
assertEquals(PathMigrationResult.Started, client.triggerPathMigration(100L, 1_000L))
|
||||
assertContentEquals(cidSeq1, client.destinationConnectionId.bytes)
|
||||
val out1 = drainOutbound(client, nowMillis = 100L)!!
|
||||
val frames1 = pipe.decryptClientApplicationFrames(out1)!!
|
||||
val challenge1 = frames1.first { it is PathChallengeFrame } as PathChallengeFrame
|
||||
// Exactly ONE RETIRE_CONNECTION_ID frame in this packet,
|
||||
// for the prior active seq (0). No retire of seq 1.
|
||||
val retires1 = frames1.filterIsInstance<RetireConnectionIdFrame>()
|
||||
assertEquals(1, retires1.size, "rotation #1 must queue exactly one RETIRE — got ${retires1.map { it.sequenceNumber }}")
|
||||
assertEquals(0L, retires1.single().sequenceNumber)
|
||||
// PATH_RESPONSE arrives — validation succeeds.
|
||||
feedDatagram(
|
||||
client,
|
||||
pipe.buildServerApplicationDatagram(listOf(PathResponseFrame(challenge1.data.copyOf())))!!,
|
||||
nowMillis = 200L,
|
||||
)
|
||||
assertContentEquals(cidSeq1, client.destinationConnectionId.bytes)
|
||||
assertEquals(1L, client.pathValidator.activeCidSequence)
|
||||
assertEquals(1L, client.pathValidator.successfulValidations)
|
||||
// Drain the ACK so the next outbound is rotation-specific.
|
||||
drainOutbound(client, nowMillis = 200L)
|
||||
|
||||
// ── Rotation #2 ────────────────────────────────────────
|
||||
assertEquals(PathMigrationResult.Started, client.triggerPathMigration(300L, 1_000L))
|
||||
assertContentEquals(cidSeq2, client.destinationConnectionId.bytes)
|
||||
val out2 = drainOutbound(client, nowMillis = 300L)!!
|
||||
val frames2 = pipe.decryptClientApplicationFrames(out2)!!
|
||||
val challenge2 = frames2.first { it is PathChallengeFrame } as PathChallengeFrame
|
||||
assertNotEquals(
|
||||
challenge1.data.toList(),
|
||||
challenge2.data.toList(),
|
||||
"second rotation must emit a fresh random PATH_CHALLENGE payload",
|
||||
)
|
||||
// Exactly ONE RETIRE_CONNECTION_ID frame for seq=1 (the
|
||||
// prior active). Critically NOT also seq=0 — that was
|
||||
// already retired in rotation #1 and the writer must
|
||||
// not re-emit a stale entry.
|
||||
val retires2 = frames2.filterIsInstance<RetireConnectionIdFrame>()
|
||||
assertEquals(1, retires2.size, "rotation #2 must queue exactly one RETIRE — got ${retires2.map { it.sequenceNumber }}")
|
||||
assertEquals(1L, retires2.single().sequenceNumber)
|
||||
|
||||
// PATH_RESPONSE arrives — second validation succeeds.
|
||||
feedDatagram(
|
||||
client,
|
||||
pipe.buildServerApplicationDatagram(listOf(PathResponseFrame(challenge2.data.copyOf())))!!,
|
||||
nowMillis = 400L,
|
||||
)
|
||||
assertContentEquals(cidSeq2, client.destinationConnectionId.bytes)
|
||||
assertEquals(2L, client.pathValidator.activeCidSequence)
|
||||
assertEquals(2L, client.pathValidator.successfulValidations)
|
||||
assertEquals(QuicConnection.Status.CONNECTED, client.status)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun validationTimeoutWithSpareRotatesDcidAndRetiresFailedSeq() =
|
||||
runBlocking {
|
||||
// Rebind-port regression: when validation times out and
|
||||
// a spare CID exists, the writer's destinationConnectionId
|
||||
// MUST rotate to that spare BEFORE the next outbound is
|
||||
// built. Otherwise we'd queue RETIRE for the failed seq
|
||||
// and stamp the same retired seq on the same packet —
|
||||
// exactly the PROTOCOL_VIOLATION the strict servers
|
||||
// close us on.
|
||||
val (client, pipe) = newConnectedClient()
|
||||
val cidSeq1 = ByteArray(8) { (0xA0 or it).toByte() }
|
||||
val cidSeq2 = ByteArray(8) { (0xB0 or it).toByte() }
|
||||
|
||||
feedDatagram(
|
||||
client,
|
||||
pipe.buildServerApplicationDatagram(
|
||||
listOf(
|
||||
NewConnectionIdFrame(1L, 0L, cidSeq1, ByteArray(16) { 0x11 }),
|
||||
NewConnectionIdFrame(2L, 0L, cidSeq2, ByteArray(16) { 0x22 }),
|
||||
),
|
||||
)!!,
|
||||
nowMillis = 0L,
|
||||
)
|
||||
drainOutbound(client, nowMillis = 0L)
|
||||
|
||||
// Trigger validation. tryStart consumes seq=1; pool keeps seq=2.
|
||||
val pto = 1_000L
|
||||
assertEquals(PathMigrationResult.Started, client.triggerPathMigration(100L, pto))
|
||||
assertContentEquals(cidSeq1, client.destinationConnectionId.bytes)
|
||||
// Drain the PATH_CHALLENGE so any subsequent build is
|
||||
// post-timeout traffic only.
|
||||
drainOutbound(client, nowMillis = 100L)
|
||||
|
||||
// Force the 3*PTO timeout. checkPathValidationTimeoutLocked
|
||||
// is the same path the driver / writer take, so this
|
||||
// exercises the production wrapper.
|
||||
val timedOut =
|
||||
client.streamsLock.withLock {
|
||||
client.checkPathValidationTimeoutLocked(nowMillis = 100L + pto * 4L)
|
||||
}
|
||||
assertTrue(timedOut)
|
||||
// Validator rotated active to the spare; connection swapped DCID.
|
||||
assertEquals(2L, client.pathValidator.activeCidSequence)
|
||||
assertContentEquals(
|
||||
cidSeq2,
|
||||
client.destinationConnectionId.bytes,
|
||||
"DCID must rotate to the spare before any outbound runs",
|
||||
)
|
||||
assertTrue(
|
||||
client.pathValidator.pendingRetireSequences.contains(1L),
|
||||
"failed seq=1 must be queued for RETIRE alongside the rotation",
|
||||
)
|
||||
|
||||
// Next outbound must stamp the SPARE seq (cidSeq2), NOT
|
||||
// the failed seq (cidSeq1) — the bug we're guarding
|
||||
// against. The packet carries RETIRE(1) on a DCID of
|
||||
// seq=2's bytes.
|
||||
val out = drainOutbound(client, nowMillis = 100L + pto * 4L)!!
|
||||
val frames = pipe.decryptClientApplicationFrames(out)!!
|
||||
val retire = frames.firstOrNull { it is RetireConnectionIdFrame } as? RetireConnectionIdFrame
|
||||
assertTrue(retire != null, "outbound must carry RETIRE for the failed seq — got ${frames.map { it::class.simpleName }}")
|
||||
assertEquals(1L, retire.sequenceNumber)
|
||||
// The pipe parser uses serverScid.length to peel the
|
||||
// DCID off the short header. We verify the rotation
|
||||
// structurally via destinationConnectionId rather than
|
||||
// reparsing the wire bytes — but the pipe successfully
|
||||
// decrypted the packet, which already requires the DCID
|
||||
// to match a recognized issued CID on the server side.
|
||||
}
|
||||
|
||||
@Test
|
||||
fun validationTimeoutWithoutSpareKeepsActiveCidAndDoesNotRetire() =
|
||||
runBlocking {
|
||||
// Rebind-port regression (no-spare branch): when
|
||||
// validation times out and NO spare exists, the
|
||||
// validator must NOT queue retire for the failed seq —
|
||||
// doing so would have us stamping a CID we just told
|
||||
// the peer to retire. Instead activeCidSequence stays
|
||||
// on the failed seq; the writer keeps using it; the
|
||||
// next NEW_CONNECTION_ID arrival lets a fresh trigger
|
||||
// rotate cleanly.
|
||||
val (client, pipe) = newConnectedClient()
|
||||
val cidSeq1 = ByteArray(8) { (0xC0 or it).toByte() }
|
||||
feedDatagram(
|
||||
client,
|
||||
pipe.buildServerApplicationDatagram(
|
||||
listOf(NewConnectionIdFrame(1L, 0L, cidSeq1, ByteArray(16) { 0x33 })),
|
||||
)!!,
|
||||
nowMillis = 0L,
|
||||
)
|
||||
drainOutbound(client, nowMillis = 0L)
|
||||
|
||||
val pto = 1_000L
|
||||
assertEquals(PathMigrationResult.Started, client.triggerPathMigration(100L, pto))
|
||||
assertContentEquals(cidSeq1, client.destinationConnectionId.bytes)
|
||||
// Pool is now empty — only seq=1 was offered, and it's
|
||||
// the active.
|
||||
assertEquals(0, client.pathValidator.unusedCount())
|
||||
drainOutbound(client, nowMillis = 100L)
|
||||
|
||||
val timedOut =
|
||||
client.streamsLock.withLock {
|
||||
client.checkPathValidationTimeoutLocked(nowMillis = 100L + pto * 4L)
|
||||
}
|
||||
assertTrue(timedOut)
|
||||
// Active stays on the failed seq; DCID unchanged.
|
||||
assertEquals(1L, client.pathValidator.activeCidSequence)
|
||||
assertContentEquals(
|
||||
cidSeq1,
|
||||
client.destinationConnectionId.bytes,
|
||||
"no spare available — DCID must stay on the failed seq, not be cleared",
|
||||
)
|
||||
// Crucially: no retire queued. The writer doesn't have
|
||||
// a way to stamp anything else, so retiring would put
|
||||
// us into the bug.
|
||||
assertTrue(
|
||||
!client.pathValidator.pendingRetireSequences.contains(1L),
|
||||
"failed seq must NOT be queued for retire when no spare exists — pending=${client.pathValidator.pendingRetireSequences}",
|
||||
)
|
||||
assertEquals(QuicConnection.Status.CONNECTED, client.status, "stuck-on-failed-cid must not close the connection")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun triggerPathMigrationBeforeHandshakeReturnsNotConnected() =
|
||||
runBlocking {
|
||||
|
||||
@@ -23,7 +23,6 @@ package com.vitorpamplona.quic.connection
|
||||
import kotlin.test.Test
|
||||
import kotlin.test.assertContentEquals
|
||||
import kotlin.test.assertEquals
|
||||
import kotlin.test.assertNull
|
||||
import kotlin.test.assertTrue
|
||||
|
||||
/**
|
||||
@@ -191,31 +190,88 @@ class PathValidatorTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
fun twoConsecutiveFailedValidationsRetireAllAbandonedSequences() {
|
||||
// Bug-B regression: prior to the fix, two failed validations
|
||||
// would leave the original seq=0 unretired (the priorSeq
|
||||
// queue entry was scheduled inside applyPathResponse, which
|
||||
// never runs for failed validations).
|
||||
fun twoConsecutiveFailedValidationsRetireAllAbandonedSequencesWithSpare() {
|
||||
// Bug-B regression (original): prior to the priorSeq fix,
|
||||
// two failed validations would leave seq=0 unretired forever
|
||||
// (the priorSeq retire was scheduled inside applyPathResponse
|
||||
// which never ran for failed validations).
|
||||
//
|
||||
// 2026-05-08 update: timeout no longer queues retire for the
|
||||
// failed seq UNLESS a spare is available to rotate the writer
|
||||
// onto. Otherwise we'd be stamping a CID we just retired
|
||||
// (the rebind-port bug). With a spare per attempt the
|
||||
// original Bug-B contract still holds — every abandoned seq
|
||||
// gets retired.
|
||||
val v = PathValidator(challengePayloadFactory = { ByteArray(8) { 0x22 } })
|
||||
// Need 4 spares: each failed validation consumes one (the
|
||||
// tryStart pick) and rotates to another (the timeout
|
||||
// recovery pick). Two failed cycles → 4 spares total.
|
||||
v.recordPeerNewConnectionId(1L, 0L, ByteArray(8) { 0x01 }, ByteArray(16) { 0x10 })
|
||||
v.recordPeerNewConnectionId(2L, 0L, ByteArray(8) { 0x02 }, ByteArray(16) { 0x20 })
|
||||
v.recordPeerNewConnectionId(3L, 0L, ByteArray(8) { 0x03 }, ByteArray(16) { 0x30 })
|
||||
v.recordPeerNewConnectionId(4L, 0L, ByteArray(8) { 0x04 }, ByteArray(16) { 0x40 })
|
||||
val pto = 1_000L
|
||||
// First attempt fails by timeout.
|
||||
// First attempt: tryStart consumes seq=1, queues retire(0),
|
||||
// active=1. Timeout consumes seq=2 (spare), queues retire(1),
|
||||
// active=2.
|
||||
v.tryStartValidation(0L, pto)
|
||||
v.checkValidationTimeout(nowMillis = pto * 4L)
|
||||
val first = v.checkValidationTimeout(nowMillis = pto * 4L)
|
||||
assertTrue(first is PathValidator.TimeoutOutcome.RecoveredOnSpare, "got $first")
|
||||
assertEquals(2L, first.newSequence)
|
||||
v.acknowledgeTerminal()
|
||||
// Second attempt also fails.
|
||||
// Second attempt: tryStart consumes seq=3, queues retire(2),
|
||||
// active=3. Timeout consumes seq=4 (spare), queues retire(3),
|
||||
// active=4.
|
||||
v.tryStartValidation(nowMillis = 10_000L, currentPtoMillis = pto)
|
||||
v.checkValidationTimeout(nowMillis = 10_000L + pto * 4L)
|
||||
val second = v.checkValidationTimeout(nowMillis = 10_000L + pto * 4L)
|
||||
assertTrue(second is PathValidator.TimeoutOutcome.RecoveredOnSpare, "got $second")
|
||||
assertEquals(4L, second.newSequence)
|
||||
|
||||
// All three sequences abandoned along the way (the original
|
||||
// seq=0 plus seq=1 plus seq=2) MUST be queued for retire.
|
||||
for (seq in listOf(0L, 1L, 2L)) {
|
||||
// Every abandoned sequence (0, 1, 2, 3) MUST be queued for
|
||||
// retire. seq=4 is the new active and is NOT queued.
|
||||
for (seq in listOf(0L, 1L, 2L, 3L)) {
|
||||
assertTrue(
|
||||
v.pendingRetireSequences.contains(seq),
|
||||
"seq=$seq abandoned but not queued for retire — pending=${v.pendingRetireSequences}",
|
||||
)
|
||||
}
|
||||
assertTrue(
|
||||
!v.pendingRetireSequences.contains(4L),
|
||||
"seq=4 is the new active CID — must NOT be queued for retire",
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun timeoutWithoutSpareKeepsFailedSeqActiveAndDoesNotRetire() {
|
||||
// Rebind-port regression: queuing a RETIRE_CONNECTION_ID for
|
||||
// the failed seq while the writer is still stamping that
|
||||
// same seq as DCID is a PROTOCOL_VIOLATION on the wire (the
|
||||
// strict server retires its routing entry as soon as it
|
||||
// sees our RETIRE, then closes us when subsequent packets
|
||||
// arrive stamped with the just-retired CID). When no spare
|
||||
// is available, the validator must NOT queue retire and
|
||||
// must keep the failed seq as activeCidSequence so the
|
||||
// writer doesn't stamp a retired CID.
|
||||
val v = PathValidator(challengePayloadFactory = { ByteArray(8) { 0x55 } })
|
||||
v.recordPeerNewConnectionId(1L, 0L, ByteArray(8) { 0x01 }, ByteArray(16) { 0x10 })
|
||||
val pto = 1_000L
|
||||
v.tryStartValidation(nowMillis = 0L, currentPtoMillis = pto)
|
||||
// After tryStartValidation pool is empty (the only spare was
|
||||
// consumed). Now timeout with no spare to recover onto.
|
||||
val outcome = v.checkValidationTimeout(nowMillis = pto * 4L)
|
||||
assertTrue(outcome is PathValidator.TimeoutOutcome.StuckOnFailedCid, "got $outcome")
|
||||
assertEquals(1L, outcome.abandoned.newCidSequence)
|
||||
assertTrue(v.state is PathValidationState.Failed)
|
||||
assertEquals(1L, v.failedValidations)
|
||||
// Critical: seq=1 is the writer's active and must NOT be
|
||||
// queued for retire.
|
||||
assertTrue(
|
||||
!v.pendingRetireSequences.contains(1L),
|
||||
"failed seq must NOT be queued for retire when no spare exists — pending=${v.pendingRetireSequences}",
|
||||
)
|
||||
// activeCidSequence stays on the failed seq so the writer
|
||||
// continues to stamp it.
|
||||
assertEquals(1L, v.activeCidSequence)
|
||||
}
|
||||
|
||||
@Test
|
||||
@@ -291,27 +347,38 @@ class PathValidatorTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
fun validationTimeoutAfter3PtoTransitionsToFailedAndRetiresFailedCid() {
|
||||
// Bug-2 regression: on 3 * PTO timeout the failed CID's
|
||||
// sequence MUST be queued for RETIRE_CONNECTION_ID.
|
||||
// Without this the peer keeps the routing entry forever
|
||||
// because we sent a packet with that DCID (the challenge)
|
||||
// but never told them we abandoned it.
|
||||
fun validationTimeoutAfter3PtoTransitionsToFailedAndRetiresFailedCidWithSpare() {
|
||||
// Bug-2 regression (updated 2026-05-08): on 3 * PTO timeout
|
||||
// the failed CID's sequence MUST be queued for
|
||||
// RETIRE_CONNECTION_ID **and** the writer must rotate off
|
||||
// that CID. The pre-2026-05-08 shape queued retire alone,
|
||||
// leaving the writer stamping the just-retired CID — see
|
||||
// [timeoutWithoutSpareKeepsFailedSeqActiveAndDoesNotRetire]
|
||||
// for the no-spare branch and the rebind-port plan for the
|
||||
// PROTOCOL_VIOLATION the strict servers raised.
|
||||
val v = PathValidator(challengePayloadFactory = { ByteArray(8) { 0x44 } })
|
||||
v.recordPeerNewConnectionId(1L, 0L, ByteArray(8) { 0x01 }, ByteArray(16))
|
||||
v.recordPeerNewConnectionId(2L, 0L, ByteArray(8) { 0x02 }, ByteArray(16) { 0x22 })
|
||||
val pto = 1_000L
|
||||
v.tryStartValidation(nowMillis = 0L, currentPtoMillis = pto)
|
||||
|
||||
assertNull(v.checkValidationTimeout(nowMillis = pto * 2L), "still within budget")
|
||||
val abandoned = v.checkValidationTimeout(nowMillis = pto * 4L)
|
||||
assertTrue(abandoned != null, "validation must time out at 3*PTO")
|
||||
assertEquals(1L, abandoned.newCidSequence)
|
||||
assertEquals(
|
||||
PathValidator.TimeoutOutcome.NotTimedOut,
|
||||
v.checkValidationTimeout(nowMillis = pto * 2L),
|
||||
"still within budget",
|
||||
)
|
||||
val outcome = v.checkValidationTimeout(nowMillis = pto * 4L)
|
||||
assertTrue(outcome is PathValidator.TimeoutOutcome.RecoveredOnSpare, "got $outcome")
|
||||
assertEquals(1L, outcome.abandoned.newCidSequence)
|
||||
assertEquals(2L, outcome.newSequence)
|
||||
assertContentEquals(ByteArray(8) { 0x02 }, outcome.newConnectionId)
|
||||
assertTrue(v.state is PathValidationState.Failed)
|
||||
assertEquals(1L, v.failedValidations)
|
||||
assertTrue(
|
||||
v.pendingRetireSequences.contains(1L),
|
||||
"abandoned CID sequence must be queued for RETIRE_CONNECTION_ID — pending=${v.pendingRetireSequences}",
|
||||
)
|
||||
assertEquals(2L, v.activeCidSequence, "active must rotate to the spare so writer stops stamping the retired CID")
|
||||
}
|
||||
|
||||
@Test
|
||||
|
||||
Reference in New Issue
Block a user