fix(quic): audit fixes for client path validation + DCID rotation

Addresses seven bugs surfaced by post-landing audit of the path
validation feature.

Spec fixes (RFC 9000 §9):
  - Bug 1: PATH_CHALLENGE was going out on the OLD DCID because the
    writer reads conn.destinationConnectionId per packet and the
    rotation only happened on PATH_RESPONSE arrival. Now rotate the
    DCID inside triggerPathMigrationLocked (abrupt-migration model
    appropriate for the "old path looks dead" trigger condition).
    Fixes the headline feature — without this the challenge cannot
    actually exercise the new path.
  - Bug 2: 3 * PTO timeout dropped the failed CID without queuing a
    RETIRE_CONNECTION_ID. The peer kept the routing entry forever.
    checkValidationTimeout now queues the failed sequence per §5.1.2.
  - Bug 3: RETIRE_CONNECTION_ID for seq 0 was silently honored. We
    have no replacement SCID to give the peer (we don't issue our
    own NEW_CONNECTION_ID frames), so the connection is unusable.
    Close with INTERNAL_ERROR instead.
  - Bug 4: triggerPathMigration had no handshake-confirmed gate;
    §9.1 forbids migration before handshake confirmation. Returns
    new PathMigrationResult.NotConnected when status != CONNECTED.

Implementation fixes:
  - Bug 5: driver was calling Clock.System.now() directly instead
    of conn.nowMillis(), breaking virtual-clock tests.
  - Bug 6: PTO threshold check ran BEFORE the consecutive-PTO
    counter increment, so threshold=2 actually required 3 PTOs.
    Increment first; threshold semantics now match the constant.
  - Bug 7: applyPeerPathResponseLocked didn't reset
    consecutivePtoCount on successful validation; the next sleep
    inherited a stale exponential-backoff multiplier even though
    the peer just proved liveness.

Code quality:
  - Rename ValidationOutcome.Validated.newConnectionIdBytes →
    connectionId; PathValidationState.Validating.newCidBytes →
    newConnectionId. The "Bytes" suffix was redundant.
  - Drop unused PathValidator(initialActiveCidSequence) parameter.
  - Drop dead coerceAtLeast(2) in pool size calculation.
  - Make pendingChallenges and pendingRetireSequences internal.
  - Fix stale KDoc references (activatePendingValidatedCid,
    forceRetireActiveIfNeeded, "retirePriorTo decreased" — none
    survived the §19.15 clamp fix).
  - PathValidator.RecordResult: drop RetirePriorToRegressed
    enum value (clamped, never returned).
  - Surface qlogObserver.onConnectionIdRetired in both the
    success and timeout paths.

Tests:
  - ClientPathMigrationTest: existing fullMigrationRoundTrip
    test now asserts DCID rotates AT challenge time, not on
    PATH_RESPONSE.
  - New retireConnectionIdForSequenceZeroClosesConnection.
  - New pathResponseSuccessResetsConsecutivePtoCount.
  - New triggerPathMigrationBeforeHandshakeReturnsNotConnected.
  - PathValidatorTest:
    validationTimeoutAfter3PtoTransitionsToFailedAndRetiresFailedCid
    now asserts the failed sequence is queued for retire.
  - retirePriorToRegressionIsRejected → renamed to
    retirePriorToRegressionIsClampedNotRejected.

All :quic:jvmTest and :nestsClient:jvmTest pass.

https://claude.ai/code/session_01PVVhSQXvw4K4oQ46FzpgaT
This commit is contained in:
Claude
2026-05-08 19:37:33 +00:00
parent 435c49bae9
commit 9b9ede2e1e
5 changed files with 284 additions and 95 deletions
@@ -83,7 +83,7 @@ sealed class PathValidationState {
data class Validating( data class Validating(
val challengeData: ByteArray, val challengeData: ByteArray,
val newCidSequence: Long, val newCidSequence: Long,
val newCidBytes: ByteArray, val newConnectionId: ByteArray,
val priorCidSequence: Long, val priorCidSequence: Long,
val startedAtMillis: Long, val startedAtMillis: Long,
val priorPtoMillis: Long, val priorPtoMillis: Long,
@@ -93,7 +93,7 @@ sealed class PathValidationState {
if (other !is Validating) return false if (other !is Validating) return false
return challengeData.contentEquals(other.challengeData) && return challengeData.contentEquals(other.challengeData) &&
newCidSequence == other.newCidSequence && newCidSequence == other.newCidSequence &&
newCidBytes.contentEquals(other.newCidBytes) && newConnectionId.contentEquals(other.newConnectionId) &&
priorCidSequence == other.priorCidSequence && priorCidSequence == other.priorCidSequence &&
startedAtMillis == other.startedAtMillis && startedAtMillis == other.startedAtMillis &&
priorPtoMillis == other.priorPtoMillis priorPtoMillis == other.priorPtoMillis
@@ -102,7 +102,7 @@ sealed class PathValidationState {
override fun hashCode(): Int { override fun hashCode(): Int {
var h = challengeData.contentHashCode() var h = challengeData.contentHashCode()
h = 31 * h + newCidSequence.hashCode() h = 31 * h + newCidSequence.hashCode()
h = 31 * h + newCidBytes.contentHashCode() h = 31 * h + newConnectionId.contentHashCode()
h = 31 * h + priorCidSequence.hashCode() h = 31 * h + priorCidSequence.hashCode()
h = 31 * h + startedAtMillis.hashCode() h = 31 * h + startedAtMillis.hashCode()
h = 31 * h + priorPtoMillis.hashCode() h = 31 * h + priorPtoMillis.hashCode()
@@ -137,6 +137,14 @@ enum class PathMigrationResult {
* pool. Caller should wait for fresh peer offers. * pool. Caller should wait for fresh peer offers.
*/ */
NoSpareCid, NoSpareCid,
/**
* Connection is not yet in CONNECTED state — RFC 9000 §9.1
* forbids initiating migration before the handshake is
* confirmed. Caller should drop the trigger; a fresh handshake
* doesn't need migration.
*/
NotConnected,
} }
/** /**
@@ -171,15 +179,6 @@ enum class PathMigrationResult {
* are plain mutable collections. * are plain mutable collections.
*/ */
class PathValidator( class PathValidator(
/**
* Sequence number of the CID the writer is currently stamping
* into outbound short-header packets. Initial value 0 per RFC
* 9000 §5.1.1 — the DCID negotiated at handshake. After a
* successful validation [activeCidSequence] is the new
* sequence; the prior sequence is in `pendingRetireSequences`
* waiting to ride out as a RETIRE_CONNECTION_ID.
*/
initialActiveCidSequence: Long = 0L,
/** /**
* Cap on the number of unused CIDs we'll buffer. Defaults to * Cap on the number of unused CIDs we'll buffer. Defaults to
* the peer's `active_connection_id_limit` transport parameter, * the peer's `active_connection_id_limit` transport parameter,
@@ -222,7 +221,15 @@ class PathValidator(
var retirePriorToWatermark: Long = 0L var retirePriorToWatermark: Long = 0L
private set private set
var activeCidSequence: Long = initialActiveCidSequence /**
* Sequence number of the CID the writer is currently stamping
* into outbound short-header packets. Starts at 0 per RFC 9000
* §5.1.1 — the DCID negotiated at handshake is implicitly
* sequence 0. Bumped to the new sequence after a successful
* [applyPathResponse]; the prior sequence is queued for
* RETIRE_CONNECTION_ID via [pendingRetireSequences].
*/
var activeCidSequence: Long = 0L
private set private set
/** /**
@@ -231,7 +238,7 @@ class PathValidator(
* next outbound application packet. Re-populated by the loss * next outbound application packet. Re-populated by the loss
* dispatcher if our RETIRE_CONNECTION_ID was lost. * dispatcher if our RETIRE_CONNECTION_ID was lost.
*/ */
val pendingRetireSequences: ArrayDeque<Long> = ArrayDeque() internal val pendingRetireSequences: ArrayDeque<Long> = ArrayDeque()
/** /**
* Outbound `PATH_CHALLENGE` payloads the writer should drain on * Outbound `PATH_CHALLENGE` payloads the writer should drain on
@@ -240,7 +247,7 @@ class PathValidator(
* (the writer also records a [com.vitorpamplona.quic.connection.recovery.RecoveryToken.PathChallenge] * (the writer also records a [com.vitorpamplona.quic.connection.recovery.RecoveryToken.PathChallenge]
* so the loss dispatcher can decide to retransmit or abandon). * so the loss dispatcher can decide to retransmit or abandon).
*/ */
val pendingChallenges: ArrayDeque<ByteArray> = ArrayDeque() internal val pendingChallenges: ArrayDeque<ByteArray> = ArrayDeque()
var state: PathValidationState = PathValidationState.Idle var state: PathValidationState = PathValidationState.Idle
private set private set
@@ -276,25 +283,31 @@ class PathValidator(
* *
* The semantics: * The semantics:
* - If [retirePriorTo] is greater than [sequenceNumber], that's * - If [retirePriorTo] is greater than [sequenceNumber], that's
* a §19.15 violation (the peer can't tell us to retire its * a §19.15 frame-encoding error (the peer can't tell us to
* own brand-new CID). * retire its own brand-new CID).
* - If [retirePriorTo] decreased, that's a §5.1.2 violation. * - If [retirePriorTo] is smaller than the current
* [retirePriorToWatermark], it's clamped to the watermark
* per §19.15 ("MUST be treated as the largest one it has
* seen") — common with reordered NEW_CONNECTION_ID arrivals,
* not a peer error.
* - If the new entry would push the pool past [maxUnusedCids], * - If the new entry would push the pool past [maxUnusedCids],
* return [Result.PoolFull] — the peer over-issued. * return [RecordResult.PoolFull] — the peer over-issued.
* - If [sequenceNumber] is below the current * - If [sequenceNumber] is below the current
* [retirePriorToWatermark], the offer is already-retired and * [retirePriorToWatermark] (after clamp), the offer is
* we silently drop it (the caller MUST still queue a * already-retired and we drop it after queuing a
* RETIRE_CONNECTION_ID for it per §5.1.2). * RETIRE_CONNECTION_ID for it (§5.1.2).
* - Duplicate sequence number with matching CID/token is * - Duplicate sequence number with matching CID/token is
* idempotent ([Result.Duplicate]); a duplicate sequence with * idempotent ([RecordResult.Duplicate]); a duplicate sequence
* DIFFERENT bytes is a §19.15 violation. * with DIFFERENT bytes is a §19.15 violation.
* *
* Side effect on success: any cached entry whose sequence * Side effect on success: any cached entry whose sequence
* number is now below the new [retirePriorTo] is moved into * number is now below the new [retirePriorToWatermark] is moved
* [pendingRetireSequences] (we owe the peer a retire) and * into [pendingRetireSequences] and dropped from [unusedCids].
* dropped from [unusedCids]. If the active CID itself is * Note that this routine does NOT force-retire the active CID
* forced-retired, the active sequence is bumped (the caller * if the watermark moves past [activeCidSequence] — that's the
* must rotate the active CID — see [forceRetireActiveIfNeeded]). * caller's responsibility (it requires picking a replacement
* from the pool and rotating the writer's DCID). See
* [QuicConnection.applyPeerNewConnectionIdLocked].
*/ */
fun recordPeerNewConnectionId( fun recordPeerNewConnectionId(
sequenceNumber: Long, sequenceNumber: Long,
@@ -385,15 +398,23 @@ class PathValidator(
} }
/** /**
* Begin a fresh path-validation attempt. Picks the lowest-sequence * Begin a fresh path-validation attempt. Picks the lowest-
* unused CID, generates a random 8-byte challenge, queues both, * sequence unused CID, generates a random 8-byte challenge,
* and transitions [state] to [PathValidationState.Validating]. * queues the challenge in [pendingChallenges], and transitions
* [state] to [PathValidationState.Validating]. The selected
* CID is removed from the unused pool; on validation success
* [applyPathResponse] commits it (callers swap the writer's
* DCID), and on timeout [checkValidationTimeout] queues it
* for RETIRE_CONNECTION_ID so the peer can drop the routing
* entry.
* *
* Caller is responsible for waking the writer so the challenge * Caller is responsible for two follow-ups when this returns
* actually goes out. The writer also stamps the new DCID into * [PathMigrationResult.Started]:
* the next outbound short-header packet — see * 1. Rotate the connection's `destinationConnectionId` to
* [QuicConnection.activatePendingValidatedCid] for the post- * [PathValidationState.Validating.newConnectionId] so the
* response promotion path. * PATH_CHALLENGE packet (and subsequent traffic, per the
* abrupt-migration model) goes out on the new path.
* 2. Wake the send loop so the challenge actually leaves.
*/ */
fun tryStartValidation( fun tryStartValidation(
nowMillis: Long, nowMillis: Long,
@@ -411,7 +432,7 @@ class PathValidator(
PathValidationState.Validating( PathValidationState.Validating(
challengeData = payload, challengeData = payload,
newCidSequence = seq, newCidSequence = seq,
newCidBytes = entry.connectionId, newConnectionId = entry.connectionId,
priorCidSequence = activeCidSequence, priorCidSequence = activeCidSequence,
startedAtMillis = nowMillis, startedAtMillis = nowMillis,
priorPtoMillis = currentPtoMillis, priorPtoMillis = currentPtoMillis,
@@ -442,7 +463,7 @@ class PathValidator(
successfulValidations += 1 successfulValidations += 1
return ValidationOutcome.Validated( return ValidationOutcome.Validated(
newSequence = current.newCidSequence, newSequence = current.newCidSequence,
newConnectionIdBytes = current.newCidBytes, connectionId = current.newConnectionId,
retiredSequence = priorSeq, retiredSequence = priorSeq,
) )
} }
@@ -454,20 +475,20 @@ class PathValidator(
data class Validated( data class Validated(
val newSequence: Long, val newSequence: Long,
val newConnectionIdBytes: ByteArray, val connectionId: ByteArray,
val retiredSequence: Long, val retiredSequence: Long,
) : ValidationOutcome() { ) : ValidationOutcome() {
override fun equals(other: Any?): Boolean { override fun equals(other: Any?): Boolean {
if (this === other) return true if (this === other) return true
if (other !is Validated) return false if (other !is Validated) return false
return newSequence == other.newSequence && return newSequence == other.newSequence &&
newConnectionIdBytes.contentEquals(other.newConnectionIdBytes) && connectionId.contentEquals(other.connectionId) &&
retiredSequence == other.retiredSequence retiredSequence == other.retiredSequence
} }
override fun hashCode(): Int { override fun hashCode(): Int {
var h = newSequence.hashCode() var h = newSequence.hashCode()
h = 31 * h + newConnectionIdBytes.contentHashCode() h = 31 * h + connectionId.contentHashCode()
h = 31 * h + retiredSequence.hashCode() h = 31 * h + retiredSequence.hashCode()
return h return h
} }
@@ -481,12 +502,21 @@ class PathValidator(
* surface a qlog event / decide whether to attempt another * surface a qlog event / decide whether to attempt another
* rotation), or null if no validation is in progress / the * rotation), or null if no validation is in progress / the
* timer hasn't fired yet. * timer hasn't fired yet.
*
* 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.
*/ */
fun checkValidationTimeout(nowMillis: Long): PathValidationState.Validating? { fun checkValidationTimeout(nowMillis: Long): PathValidationState.Validating? {
val current = state as? PathValidationState.Validating ?: return null val current = state as? PathValidationState.Validating ?: return null
val elapsed = nowMillis - current.startedAtMillis val elapsed = nowMillis - current.startedAtMillis
val budget = (current.priorPtoMillis * VALIDATION_PTO_BUDGET_MULTIPLIER).coerceAtLeast(MIN_VALIDATION_BUDGET_MS) val budget = (current.priorPtoMillis * VALIDATION_PTO_BUDGET_MULTIPLIER).coerceAtLeast(MIN_VALIDATION_BUDGET_MS)
if (elapsed < budget) return null if (elapsed < budget) return null
queueRetireSequence(current.newCidSequence)
state = PathValidationState.Failed state = PathValidationState.Failed
failedValidations += 1 failedValidations += 1
return current return current
@@ -519,11 +519,15 @@ class QuicConnection(
*/ */
internal val pathValidator: PathValidator = internal val pathValidator: PathValidator =
PathValidator( PathValidator(
initialActiveCidSequence = 0L, // Cap the unused-CID buffer at the smaller of our own
// [activeConnectionIdLimit] (which we advertised to the
// peer in transport parameters) and the validator's hard
// [PathValidator.DEFAULT_MAX_UNUSED_CIDS] memory cap. A
// peer that issues more CIDs than we said we'd accept
// hits the cap and excess offers are dropped.
maxUnusedCids = maxUnusedCids =
config.activeConnectionIdLimit config.activeConnectionIdLimit
.coerceAtMost(PathValidator.DEFAULT_MAX_UNUSED_CIDS.toLong()) .coerceAtMost(PathValidator.DEFAULT_MAX_UNUSED_CIDS.toLong())
.coerceAtLeast(2L)
.toInt(), .toInt(),
) )
@@ -2000,60 +2004,99 @@ class QuicConnection(
} }
is PathValidator.ValidationOutcome.Validated -> { is PathValidator.ValidationOutcome.Validated -> {
destinationConnectionId = ConnectionId(outcome.newConnectionIdBytes) // Bug-7 fix: a valid PATH_RESPONSE proves the peer
// can reach us on the new path. Reset the PTO
// backoff so the send loop returns to baseline
// pacing instead of carrying a stale "many PTOs
// expired" multiplier into healthy traffic.
consecutivePtoCount = 0
qlogObserver.onPathValidationSucceeded(outcome.newSequence) qlogObserver.onPathValidationSucceeded(outcome.newSequence)
qlogObserver.onConnectionIdActivated("peer", outcome.newSequence, outcome.newConnectionIdBytes) qlogObserver.onConnectionIdActivated("peer", outcome.newSequence, outcome.connectionId)
qlogObserver.onConnectionIdRetired("peer", outcome.retiredSequence)
pathValidator.acknowledgeTerminal() pathValidator.acknowledgeTerminal()
// Note: the writer's `destinationConnectionId` was
// already swapped to [outcome.connectionId] inside
// [triggerPathMigrationLocked] (abrupt-migration
// model — the challenge itself MUST go out on the
// new path per RFC 9000 §9.3). On success there's
// nothing more to swap; we just confirm the
// promotion is durable by re-stamping (idempotent).
destinationConnectionId = ConnectionId(outcome.connectionId)
} }
} }
} }
/** /**
* RFC 9000 §19.16: peer asked us to retire one of our own * RFC 9000 §19.16: peer asked us to retire one of our own
* source CIDs. Today we only ever issue one source CID at * source CIDs.
* connection start (sequence 0), so any RETIRE_CONNECTION_ID *
* the peer sends about a sequence > 0 references a CID we * Two cases:
* never issued — RFC 9000 says this is PROTOCOL_VIOLATION. * - Sequence > 0: we never issued anything beyond seq 0, so
* Sequence 0 retire is also a violation per §19.16: "Receipt * this references a CID we never advertised. PROTOCOL_VIOLATION
* of a RETIRE_CONNECTION_ID frame containing a sequence number * per §19.16.
* greater than any previously sent to the peer MUST be * - Sequence == 0: peer is asking us to retire the SCID we
* treated as a connection error of type PROTOCOL_VIOLATION." * picked at connection start. Per RFC 9000 §5.1.1 we'd be
* expected to have already issued a replacement via
* NEW_CONNECTION_ID and switch the peer to it. We don't
* issue our own NEW_CONNECTION_ID frames today, so we have
* no replacement to give the peer. Closing the connection
* surfaces this as a known limitation rather than continuing
* against a peer that thinks we agreed to stop using our
* only CID.
* *
* Caller must hold [streamsLock]. * Caller must hold [streamsLock].
*/ */
internal fun applyPeerRetireConnectionIdLocked(sequenceNumber: Long) { internal fun applyPeerRetireConnectionIdLocked(sequenceNumber: Long) {
// We only issue sequence 0 today; any retire request beyond
// that references a CID we never advertised. This is a
// peer-side violation per §19.16.
if (sequenceNumber > 0L) { if (sequenceNumber > 0L) {
markClosedExternally( markClosedExternally(
"PROTOCOL_VIOLATION: RETIRE_CONNECTION_ID seq=$sequenceNumber > any we issued (0)", "PROTOCOL_VIOLATION: RETIRE_CONNECTION_ID seq=$sequenceNumber > any we issued (0)",
) )
return
} }
// Sequence 0 retire today means "stop using your initial // Sequence 0: peer wants us off our initial SCID, but we
// SCID." We don't model client-issued CID rotation, so the // haven't issued any replacements. Close — see kdoc above.
// request is silently honored at the protocol level (we markClosedExternally(
// never re-use the SCID in any new long-header packet — "INTERNAL_ERROR: peer asked to retire our initial SCID but we have no replacement to offer",
// long headers are gone after handshake completion). )
} }
/** /**
* Try to start client-initiated path validation + DCID rotation * Try to start client-initiated path validation + DCID rotation
* (RFC 9000 §9). Picks the lowest-sequence unused CID from the * (RFC 9000 §9). Picks the lowest-sequence unused CID from the
* pool, queues a fresh PATH_CHALLENGE, and records the * pool, queues a fresh PATH_CHALLENGE, **swaps the writer's
* outstanding state. Returns the result so the caller can * outbound DCID to the new CID**, and records the outstanding
* decide whether to retry later (e.g. on the next PTO if the * state. Returns the result so the caller can decide whether
* pool was empty when this fired). * to retry later (e.g. on the next PTO if the pool was empty
* when this fired).
* *
* Caller must hold [streamsLock]. * Bug-1 fix — abrupt-migration model: the writer stamps a
* single connection-level [destinationConnectionId] onto every
* outbound short-header packet. To put the PATH_CHALLENGE on
* the new path (RFC 9000 §9.3) we rotate the DCID immediately
* here, not on validation success. The trigger condition
* ("path probably dead — N consecutive PTOs without ACKs")
* makes abrupt migration appropriate; we don't gain anything
* by holding the old DCID alongside. If validation later fails
* (3 * PTO timeout), [checkPathValidationTimeoutLocked]
* abandons the new CID without rolling back — by then the
* connection is in trouble and the next PTO either picks
* another spare CID or the connection idles out.
*
* Caller must hold [streamsLock]. Refuses to start before
* handshake confirmation per RFC 9000 §9.1 (returns
* [PathMigrationResult.NotConnected]).
*/ */
internal fun triggerPathMigrationLocked( internal fun triggerPathMigrationLocked(
nowMillis: Long, nowMillis: Long,
currentPtoMillis: Long, currentPtoMillis: Long,
): PathMigrationResult { ): PathMigrationResult {
if (!handshakeComplete || status != Status.CONNECTED) {
return PathMigrationResult.NotConnected
}
val result = pathValidator.tryStartValidation(nowMillis, currentPtoMillis) val result = pathValidator.tryStartValidation(nowMillis, currentPtoMillis)
if (result == PathMigrationResult.Started) { if (result == PathMigrationResult.Started) {
val validating = pathValidator.state as PathValidationState.Validating val validating = pathValidator.state as PathValidationState.Validating
destinationConnectionId = ConnectionId(validating.newConnectionId)
qlogObserver.onPathValidationStarted(validating.newCidSequence) qlogObserver.onPathValidationStarted(validating.newCidSequence)
} }
return result return result
@@ -2073,16 +2116,22 @@ class QuicConnection(
/** /**
* Check whether an outstanding PATH_CHALLENGE has exceeded the * Check whether an outstanding PATH_CHALLENGE has exceeded the
* 3 * PTO budget (RFC 9000 §8.2.4). Called from the driver's * 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. Returns true when validation just timed out.
* — caller may surface that to qlog and decide to retry with *
* another CID, or close the connection if the budget for * On timeout the validator queues the failed CID's sequence
* retries is exhausted. * 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.
* *
* Caller must hold [streamsLock]. * Caller must hold [streamsLock].
*/ */
internal fun checkPathValidationTimeoutLocked(nowMillis: Long): Boolean { internal fun checkPathValidationTimeoutLocked(nowMillis: Long): Boolean {
val abandoned = pathValidator.checkValidationTimeout(nowMillis) ?: return false val abandoned = pathValidator.checkValidationTimeout(nowMillis) ?: return false
qlogObserver.onPathValidationFailed(abandoned.newCidSequence) qlogObserver.onPathValidationFailed(abandoned.newCidSequence)
qlogObserver.onConnectionIdRetired("peer", abandoned.newCidSequence)
pathValidator.acknowledgeTerminal() pathValidator.acknowledgeTerminal()
return true return true
} }
@@ -350,6 +350,12 @@ internal suspend fun handlePtoFired(conn: QuicConnection) {
if (conn.initial.sendProtection != null && !conn.initial.keysDiscarded) { if (conn.initial.sendProtection != null && !conn.initial.keysDiscarded) {
conn.requeueAllInflightCrypto(EncryptionLevel.INITIAL) conn.requeueAllInflightCrypto(EncryptionLevel.INITIAL)
} }
// Bug-6 fix: increment BEFORE the threshold check. With the
// pre-fix ordering and threshold=2, the rotation actually fired
// on the 3rd PTO (count went 0→1→2 before check). Now the count
// matches the constant's natural reading: "after 2 consecutive
// PTOs with no progress, trigger migration on the 2nd PTO firing."
conn.consecutivePtoCount = (conn.consecutivePtoCount + 1).coerceAtMost(6)
// Once 1-RTT keys are installed, PTO must also retransmit application // Once 1-RTT keys are installed, PTO must also retransmit application
// data — STREAM bytes that were sent but never ACK'd. Without this, // data — STREAM bytes that were sent but never ACK'd. Without this,
// a single corrupted/lost 1-RTT packet (especially the first one // a single corrupted/lost 1-RTT packet (especially the first one
@@ -379,16 +385,14 @@ internal suspend fun handlePtoFired(conn: QuicConnection) {
// also dead — abandon and let the next PTO try with // also dead — abandon and let the next PTO try with
// another CID (or surface the failure to the higher // another CID (or surface the failure to the higher
// layer). // layer).
val nowMillis = //
kotlin.time.Clock.System // Bug-5 fix: use [conn.nowMillis] so a test-injected
.now() // virtual clock takes effect. The pre-fix shape called
.toEpochMilliseconds() // [Clock.System.now()] directly, ignoring the
val maxAckDelayMs = // connection's clock supplier and breaking timing
if (conn.application.sendProtection != null) { // assertions in unit tests.
conn.peerTransportParameters?.maxAckDelay ?: 0L val nowMillis = conn.nowMillis()
} else { val maxAckDelayMs = conn.peerTransportParameters?.maxAckDelay ?: 0L
0L
}
val ptoBaseMs = conn.lossDetection.ptoBaseMs(maxAckDelayMs).coerceAtLeast(1L) val ptoBaseMs = conn.lossDetection.ptoBaseMs(maxAckDelayMs).coerceAtLeast(1L)
conn.checkPathValidationTimeoutLocked(nowMillis) conn.checkPathValidationTimeoutLocked(nowMillis)
if (conn.consecutivePtoCount >= PATH_PROBE_PTO_THRESHOLD) { if (conn.consecutivePtoCount >= PATH_PROBE_PTO_THRESHOLD) {
@@ -396,7 +400,6 @@ internal suspend fun handlePtoFired(conn: QuicConnection) {
} }
} }
} }
conn.consecutivePtoCount = (conn.consecutivePtoCount + 1).coerceAtMost(6)
} }
/** /**
@@ -409,6 +412,11 @@ internal suspend fun handlePtoFired(conn: QuicConnection) {
* (single dropped packet trips a rotation); 4+ is too late (user * (single dropped packet trips a rotation); 4+ is too late (user
* notices the silence). * notices the silence).
* *
* The check in [handlePtoFired] runs AFTER the consecutive-PTO
* counter is incremented, so the threshold value is the count of
* PTOs that fired without an inbound ACK arriving in between.
* With value 2 migration triggers on the 2nd consecutive PTO.
*
* Once the threshold is crossed, [handlePtoFired] calls into * Once the threshold is crossed, [handlePtoFired] calls into
* [QuicConnection.triggerPathMigrationLocked] which is itself * [QuicConnection.triggerPathMigrationLocked] which is itself
* idempotent — a second crossing while validation is already in * idempotent — a second crossing while validation is already in
@@ -150,13 +150,21 @@ class ClientPathMigrationTest {
drainOutbound(client, nowMillis = 0L) drainOutbound(client, nowMillis = 0L)
// Step 2 — application triggers migration. The validator picks // Step 2 — application triggers migration. The validator picks
// sequence 1 and queues a PATH_CHALLENGE. // sequence 1, queues a PATH_CHALLENGE, AND swaps the writer's
// outbound DCID immediately so the challenge goes out on the
// new path (Bug-1 fix — abrupt migration model per RFC 9000
// §9.3).
val triggered = client.triggerPathMigration(nowMillis = 100L, currentPtoMillis = 1_000L) val triggered = client.triggerPathMigration(nowMillis = 100L, currentPtoMillis = 1_000L)
assertEquals(PathMigrationResult.Started, triggered) assertEquals(PathMigrationResult.Started, triggered)
assertTrue(client.pathValidator.state is PathValidationState.Validating) assertTrue(client.pathValidator.state is PathValidationState.Validating)
assertContentEquals(
newCidBytes,
client.destinationConnectionId.bytes,
"DCID must rotate at challenge time so PATH_CHALLENGE goes out on the new path",
)
// Step 3 — the next outbound application packet must carry a // Step 3 — the next outbound application packet must carry a
// PATH_CHALLENGE. // PATH_CHALLENGE stamped with the NEW DCID.
val challengeOut = drainOutbound(client, nowMillis = 100L) val challengeOut = drainOutbound(client, nowMillis = 100L)
assertTrue(challengeOut != null, "client must emit a packet carrying PATH_CHALLENGE") assertTrue(challengeOut != null, "client must emit a packet carrying PATH_CHALLENGE")
val outboundFrames = pipe.decryptClientApplicationFrames(challengeOut) val outboundFrames = pipe.decryptClientApplicationFrames(challengeOut)
@@ -171,9 +179,11 @@ class ClientPathMigrationTest {
)!! )!!
feedDatagram(client, response, nowMillis = 200L) feedDatagram(client, response, nowMillis = 200L)
// Step 5 — DCID must have rotated to the new bytes; the prior // Step 5 — validation succeeded; DCID is still the new bytes
// sequence (0) must be queued for RETIRE_CONNECTION_ID. // (already rotated at step 2), the prior sequence (0) is
assertContentEquals(newCidBytes, client.destinationConnectionId.bytes, "DCID must rotate to new bytes") // queued for RETIRE_CONNECTION_ID, and activeCidSequence now
// reflects the new sequence.
assertContentEquals(newCidBytes, client.destinationConnectionId.bytes)
assertEquals(1L, client.pathValidator.activeCidSequence) assertEquals(1L, client.pathValidator.activeCidSequence)
assertTrue( assertTrue(
client.pathValidator.pendingRetireSequences.contains(0L), client.pathValidator.pendingRetireSequences.contains(0L),
@@ -197,36 +207,43 @@ class ClientPathMigrationTest {
} }
@Test @Test
fun pathResponseWithMismatchingPayloadDoesNotRotateDcid() = fun pathResponseWithMismatchingPayloadKeepsValidatingAndDcid() =
runBlocking { runBlocking {
val (client, pipe) = newConnectedClient() val (client, pipe) = newConnectedClient()
val originalDcid = client.destinationConnectionId
// Seed a spare CID and trigger migration. // Seed a spare CID and trigger migration. Under abrupt
// migration the DCID rotates at trigger time, NOT at
// PATH_RESPONSE — so the post-trigger state is "DCID is
// already the new bytes; we're awaiting validation."
val newCid = ByteArray(8) { 0x01 }
val offer = val offer =
pipe.buildServerApplicationDatagram( pipe.buildServerApplicationDatagram(
listOf( listOf(
NewConnectionIdFrame( NewConnectionIdFrame(
sequenceNumber = 1L, sequenceNumber = 1L,
retirePriorTo = 0L, retirePriorTo = 0L,
connectionId = ByteArray(8) { 0x01 }, connectionId = newCid,
statelessResetToken = ByteArray(16) { 0x10 }, statelessResetToken = ByteArray(16) { 0x10 },
), ),
), ),
)!! )!!
feedDatagram(client, offer, nowMillis = 0L) feedDatagram(client, offer, nowMillis = 0L)
client.triggerPathMigration(nowMillis = 100L, currentPtoMillis = 1_000L) client.triggerPathMigration(nowMillis = 100L, currentPtoMillis = 1_000L)
assertContentEquals(newCid, client.destinationConnectionId.bytes, "DCID rotates at challenge time")
drainOutbound(client, nowMillis = 100L) // emit challenge, drop content drainOutbound(client, nowMillis = 100L) // emit challenge, drop content
// Server replies with WRONG payload — must not rotate. // Server replies with WRONG payload — validation must NOT
// succeed and the validator state must stay in Validating
// until the 3 * PTO timeout (or a correct response).
val bogusResponse = val bogusResponse =
pipe.buildServerApplicationDatagram( pipe.buildServerApplicationDatagram(
listOf(PathResponseFrame(ByteArray(8) { 0xFF.toByte() })), listOf(PathResponseFrame(ByteArray(8) { 0xFF.toByte() })),
)!! )!!
feedDatagram(client, bogusResponse, nowMillis = 200L) feedDatagram(client, bogusResponse, nowMillis = 200L)
assertEquals(originalDcid, client.destinationConnectionId, "mismatched response must NOT rotate DCID") assertContentEquals(newCid, client.destinationConnectionId.bytes, "mismatched response must not undo the rotation")
assertTrue(client.pathValidator.state is PathValidationState.Validating, "still validating") assertTrue(client.pathValidator.state is PathValidationState.Validating, "still validating")
assertEquals(0L, client.pathValidator.activeCidSequence, "active sequence still old until validation succeeds")
} }
@Test @Test
@@ -257,4 +274,80 @@ class ClientPathMigrationTest {
"got ${client.status}", "got ${client.status}",
) )
} }
@Test
fun retireConnectionIdForSequenceZeroClosesConnection() =
runBlocking {
// Bug-3 regression: peer asking us to retire our initial
// SCID (seq 0) leaves us with no replacement to give them.
// We close rather than silently honoring — see
// [QuicConnection.applyPeerRetireConnectionIdLocked].
val (client, pipe) = newConnectedClient()
val packet = pipe.buildServerApplicationDatagram(listOf(RetireConnectionIdFrame(sequenceNumber = 0L)))!!
feedDatagram(client, packet, nowMillis = 0L)
assertTrue(
client.status == QuicConnection.Status.CLOSING || client.status == QuicConnection.Status.CLOSED,
"got ${client.status}; seq=0 retire must close because we have no replacement SCID",
)
}
@Test
fun pathResponseSuccessResetsConsecutivePtoCount() =
runBlocking {
// Bug-7 regression: a successful PATH_RESPONSE proves the
// peer can reach us on the new path. The consecutive-PTO
// counter must reset so the send loop's exponential
// backoff returns to baseline pacing instead of carrying
// a stale "many PTOs expired" multiplier.
val (client, pipe) = newConnectedClient()
val newCid = ByteArray(8) { 0x55 }
val token = ByteArray(16) { 0x66 }
feedDatagram(
client,
pipe.buildServerApplicationDatagram(
listOf(NewConnectionIdFrame(1L, 0L, newCid, token)),
)!!,
nowMillis = 0L,
)
drainOutbound(client, nowMillis = 0L)
// Pretend we burned through 4 PTOs without progress.
client.consecutivePtoCount = 4
client.triggerPathMigration(nowMillis = 100L, currentPtoMillis = 1_000L)
val challengeOut = drainOutbound(client, nowMillis = 100L)!!
val challenge =
pipe.decryptClientApplicationFrames(challengeOut)!!.first { it is PathChallengeFrame }
as PathChallengeFrame
// PATH_RESPONSE arrives with matching payload — Bug-7 fix
// resets the counter inside applyPeerPathResponseLocked.
feedDatagram(
client,
pipe.buildServerApplicationDatagram(listOf(PathResponseFrame(challenge.data.copyOf())))!!,
nowMillis = 200L,
)
assertEquals(0, client.consecutivePtoCount, "successful path validation must reset consecutivePtoCount")
}
@Test
fun triggerPathMigrationBeforeHandshakeReturnsNotConnected() =
runBlocking {
// Bug-4 regression: RFC 9000 §9.1 forbids initiating
// migration before the handshake is confirmed. The
// public API must refuse rather than silently misbehave.
val client =
QuicConnection(
serverName = "example.test",
config = QuicConnectionConfig(),
tlsCertificateValidator =
com.vitorpamplona.quic.tls
.PermissiveCertificateValidator(),
)
// Connection is in HANDSHAKING state; no transport
// parameters have been parsed yet.
assertEquals(QuicConnection.Status.HANDSHAKING, client.status)
val result = client.triggerPathMigration(nowMillis = 0L, currentPtoMillis = 100L)
assertEquals(PathMigrationResult.NotConnected, result)
}
} }
@@ -166,7 +166,7 @@ class PathValidatorTest {
assertTrue(outcome is PathValidator.ValidationOutcome.Validated, "got $outcome") assertTrue(outcome is PathValidator.ValidationOutcome.Validated, "got $outcome")
assertEquals(1L, outcome.newSequence) assertEquals(1L, outcome.newSequence)
assertEquals(0L, outcome.retiredSequence) assertEquals(0L, outcome.retiredSequence)
assertContentEquals(ByteArray(8) { 0xAA.toByte() }, outcome.newConnectionIdBytes) assertContentEquals(ByteArray(8) { 0xAA.toByte() }, outcome.connectionId)
assertEquals(1L, v.activeCidSequence) assertEquals(1L, v.activeCidSequence)
assertTrue(v.state is PathValidationState.Succeeded) assertTrue(v.state is PathValidationState.Succeeded)
assertTrue(v.pendingRetireSequences.contains(0L), "old sequence must be queued for RETIRE_CONNECTION_ID") assertTrue(v.pendingRetireSequences.contains(0L), "old sequence must be queued for RETIRE_CONNECTION_ID")
@@ -191,7 +191,12 @@ class PathValidatorTest {
} }
@Test @Test
fun validationTimeoutAfter3PtoTransitionsToFailed() { 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.
val v = PathValidator(challengePayloadFactory = { ByteArray(8) { 0x44 } }) val v = PathValidator(challengePayloadFactory = { ByteArray(8) { 0x44 } })
v.recordPeerNewConnectionId(1L, 0L, ByteArray(8) { 0x01 }, ByteArray(16)) v.recordPeerNewConnectionId(1L, 0L, ByteArray(8) { 0x01 }, ByteArray(16))
val pto = 1_000L val pto = 1_000L
@@ -203,6 +208,10 @@ class PathValidatorTest {
assertEquals(1L, abandoned.newCidSequence) assertEquals(1L, abandoned.newCidSequence)
assertTrue(v.state is PathValidationState.Failed) assertTrue(v.state is PathValidationState.Failed)
assertEquals(1L, v.failedValidations) assertEquals(1L, v.failedValidations)
assertTrue(
v.pendingRetireSequences.contains(1L),
"abandoned CID sequence must be queued for RETIRE_CONNECTION_ID — pending=${v.pendingRetireSequences}",
)
} }
@Test @Test