diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/PathValidator.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/PathValidator.kt index ad820d87d..0e79d1255 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/PathValidator.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/PathValidator.kt @@ -83,7 +83,7 @@ sealed class PathValidationState { data class Validating( val challengeData: ByteArray, val newCidSequence: Long, - val newCidBytes: ByteArray, + val newConnectionId: ByteArray, val priorCidSequence: Long, val startedAtMillis: Long, val priorPtoMillis: Long, @@ -93,7 +93,7 @@ sealed class PathValidationState { if (other !is Validating) return false return challengeData.contentEquals(other.challengeData) && newCidSequence == other.newCidSequence && - newCidBytes.contentEquals(other.newCidBytes) && + newConnectionId.contentEquals(other.newConnectionId) && priorCidSequence == other.priorCidSequence && startedAtMillis == other.startedAtMillis && priorPtoMillis == other.priorPtoMillis @@ -102,7 +102,7 @@ sealed class PathValidationState { override fun hashCode(): Int { var h = challengeData.contentHashCode() h = 31 * h + newCidSequence.hashCode() - h = 31 * h + newCidBytes.contentHashCode() + h = 31 * h + newConnectionId.contentHashCode() h = 31 * h + priorCidSequence.hashCode() h = 31 * h + startedAtMillis.hashCode() h = 31 * h + priorPtoMillis.hashCode() @@ -137,6 +137,14 @@ enum class PathMigrationResult { * pool. Caller should wait for fresh peer offers. */ 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. */ 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 * the peer's `active_connection_id_limit` transport parameter, @@ -222,7 +221,15 @@ class PathValidator( var retirePriorToWatermark: Long = 0L 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 /** @@ -231,7 +238,7 @@ class PathValidator( * next outbound application packet. Re-populated by the loss * dispatcher if our RETIRE_CONNECTION_ID was lost. */ - val pendingRetireSequences: ArrayDeque = ArrayDeque() + internal val pendingRetireSequences: ArrayDeque = ArrayDeque() /** * 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] * so the loss dispatcher can decide to retransmit or abandon). */ - val pendingChallenges: ArrayDeque = ArrayDeque() + internal val pendingChallenges: ArrayDeque = ArrayDeque() var state: PathValidationState = PathValidationState.Idle private set @@ -276,25 +283,31 @@ class PathValidator( * * The semantics: * - If [retirePriorTo] is greater than [sequenceNumber], that's - * a §19.15 violation (the peer can't tell us to retire its - * own brand-new CID). - * - If [retirePriorTo] decreased, that's a §5.1.2 violation. + * a §19.15 frame-encoding error (the peer can't tell us to + * retire its own brand-new CID). + * - 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], - * return [Result.PoolFull] — the peer over-issued. + * return [RecordResult.PoolFull] — the peer over-issued. * - If [sequenceNumber] is below the current - * [retirePriorToWatermark], the offer is already-retired and - * we silently drop it (the caller MUST still queue a - * RETIRE_CONNECTION_ID for it per §5.1.2). + * [retirePriorToWatermark] (after clamp), the offer is + * already-retired and we drop it after queuing a + * RETIRE_CONNECTION_ID for it (§5.1.2). * - Duplicate sequence number with matching CID/token is - * idempotent ([Result.Duplicate]); a duplicate sequence with - * DIFFERENT bytes is a §19.15 violation. + * idempotent ([RecordResult.Duplicate]); a duplicate sequence + * with DIFFERENT bytes is a §19.15 violation. * * Side effect on success: any cached entry whose sequence - * number is now below the new [retirePriorTo] is moved into - * [pendingRetireSequences] (we owe the peer a retire) and - * dropped from [unusedCids]. If the active CID itself is - * forced-retired, the active sequence is bumped (the caller - * must rotate the active CID — see [forceRetireActiveIfNeeded]). + * number is now below the new [retirePriorToWatermark] is moved + * into [pendingRetireSequences] and dropped from [unusedCids]. + * Note that this routine does NOT force-retire the active CID + * if the watermark moves past [activeCidSequence] — that's the + * caller's responsibility (it requires picking a replacement + * from the pool and rotating the writer's DCID). See + * [QuicConnection.applyPeerNewConnectionIdLocked]. */ fun recordPeerNewConnectionId( sequenceNumber: Long, @@ -385,15 +398,23 @@ class PathValidator( } /** - * Begin a fresh path-validation attempt. Picks the lowest-sequence - * unused CID, generates a random 8-byte challenge, queues both, - * and transitions [state] to [PathValidationState.Validating]. + * Begin a fresh path-validation attempt. Picks the lowest- + * sequence unused CID, generates a random 8-byte challenge, + * 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 - * actually goes out. The writer also stamps the new DCID into - * the next outbound short-header packet — see - * [QuicConnection.activatePendingValidatedCid] for the post- - * response promotion path. + * Caller is responsible for two follow-ups when this returns + * [PathMigrationResult.Started]: + * 1. Rotate the connection's `destinationConnectionId` to + * [PathValidationState.Validating.newConnectionId] so the + * 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( nowMillis: Long, @@ -411,7 +432,7 @@ class PathValidator( PathValidationState.Validating( challengeData = payload, newCidSequence = seq, - newCidBytes = entry.connectionId, + newConnectionId = entry.connectionId, priorCidSequence = activeCidSequence, startedAtMillis = nowMillis, priorPtoMillis = currentPtoMillis, @@ -442,7 +463,7 @@ class PathValidator( successfulValidations += 1 return ValidationOutcome.Validated( newSequence = current.newCidSequence, - newConnectionIdBytes = current.newCidBytes, + connectionId = current.newConnectionId, retiredSequence = priorSeq, ) } @@ -454,20 +475,20 @@ class PathValidator( data class Validated( val newSequence: Long, - val newConnectionIdBytes: ByteArray, + val connectionId: ByteArray, val retiredSequence: Long, ) : ValidationOutcome() { override fun equals(other: Any?): Boolean { if (this === other) return true if (other !is Validated) return false return newSequence == other.newSequence && - newConnectionIdBytes.contentEquals(other.newConnectionIdBytes) && + connectionId.contentEquals(other.connectionId) && retiredSequence == other.retiredSequence } override fun hashCode(): Int { var h = newSequence.hashCode() - h = 31 * h + newConnectionIdBytes.contentHashCode() + h = 31 * h + connectionId.contentHashCode() h = 31 * h + retiredSequence.hashCode() return h } @@ -481,12 +502,21 @@ class PathValidator( * surface a qlog event / decide whether to attempt another * rotation), or null if no validation is in progress / the * 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? { val current = state as? PathValidationState.Validating ?: return null 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) state = PathValidationState.Failed failedValidations += 1 return current diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt index 4508ae504..bf198fed1 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt @@ -519,11 +519,15 @@ class QuicConnection( */ internal val 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 = config.activeConnectionIdLimit .coerceAtMost(PathValidator.DEFAULT_MAX_UNUSED_CIDS.toLong()) - .coerceAtLeast(2L) .toInt(), ) @@ -2000,60 +2004,99 @@ class QuicConnection( } 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.onConnectionIdActivated("peer", outcome.newSequence, outcome.newConnectionIdBytes) + qlogObserver.onConnectionIdActivated("peer", outcome.newSequence, outcome.connectionId) + qlogObserver.onConnectionIdRetired("peer", outcome.retiredSequence) 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 - * source CIDs. Today we only ever issue one source CID at - * connection start (sequence 0), so any RETIRE_CONNECTION_ID - * the peer sends about a sequence > 0 references a CID we - * never issued — RFC 9000 says this is PROTOCOL_VIOLATION. - * Sequence 0 retire is also a violation per §19.16: "Receipt - * of a RETIRE_CONNECTION_ID frame containing a sequence number - * greater than any previously sent to the peer MUST be - * treated as a connection error of type PROTOCOL_VIOLATION." + * source CIDs. + * + * Two cases: + * - Sequence > 0: we never issued anything beyond seq 0, so + * this references a CID we never advertised. PROTOCOL_VIOLATION + * per §19.16. + * - Sequence == 0: peer is asking us to retire the SCID we + * 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]. */ 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) { markClosedExternally( "PROTOCOL_VIOLATION: RETIRE_CONNECTION_ID seq=$sequenceNumber > any we issued (0)", ) + return } - // Sequence 0 retire today means "stop using your initial - // SCID." We don't model client-issued CID rotation, so the - // request is silently honored at the protocol level (we - // never re-use the SCID in any new long-header packet — - // long headers are gone after handshake completion). + // Sequence 0: peer wants us off our initial SCID, but we + // haven't issued any replacements. Close — see kdoc above. + markClosedExternally( + "INTERNAL_ERROR: peer asked to retire our initial SCID but we have no replacement to offer", + ) } /** * Try to start client-initiated path validation + DCID rotation * (RFC 9000 §9). Picks the lowest-sequence unused CID from the - * pool, queues a fresh PATH_CHALLENGE, and records the - * outstanding state. Returns the result so the caller can - * decide whether to retry later (e.g. on the next PTO if the - * pool was empty when this fired). + * pool, queues a fresh PATH_CHALLENGE, **swaps the writer's + * outbound DCID to the new CID**, and records the outstanding + * state. Returns the result so the caller can decide whether + * 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( nowMillis: Long, currentPtoMillis: Long, ): PathMigrationResult { + if (!handshakeComplete || status != Status.CONNECTED) { + return PathMigrationResult.NotConnected + } val result = pathValidator.tryStartValidation(nowMillis, currentPtoMillis) if (result == PathMigrationResult.Started) { val validating = pathValidator.state as PathValidationState.Validating + destinationConnectionId = ConnectionId(validating.newConnectionId) qlogObserver.onPathValidationStarted(validating.newCidSequence) } return result @@ -2073,16 +2116,22 @@ 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 - * — caller may surface that to qlog and decide to retry with - * another CID, or close the connection if the budget for - * retries is exhausted. + * PTO timer path. 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. * * 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 } diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionDriver.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionDriver.kt index 0e2dcd184..120fb9f2b 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionDriver.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionDriver.kt @@ -350,6 +350,12 @@ internal suspend fun handlePtoFired(conn: QuicConnection) { if (conn.initial.sendProtection != null && !conn.initial.keysDiscarded) { 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 // data — STREAM bytes that were sent but never ACK'd. Without this, // 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 // another CID (or surface the failure to the higher // layer). - val nowMillis = - kotlin.time.Clock.System - .now() - .toEpochMilliseconds() - val maxAckDelayMs = - if (conn.application.sendProtection != null) { - conn.peerTransportParameters?.maxAckDelay ?: 0L - } else { - 0L - } + // + // Bug-5 fix: use [conn.nowMillis] so a test-injected + // virtual clock takes effect. The pre-fix shape called + // [Clock.System.now()] directly, ignoring the + // connection's clock supplier and breaking timing + // assertions in unit tests. + val nowMillis = conn.nowMillis() + val maxAckDelayMs = conn.peerTransportParameters?.maxAckDelay ?: 0L val ptoBaseMs = conn.lossDetection.ptoBaseMs(maxAckDelayMs).coerceAtLeast(1L) conn.checkPathValidationTimeoutLocked(nowMillis) 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 * 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 * [QuicConnection.triggerPathMigrationLocked] which is itself * idempotent — a second crossing while validation is already in diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/ClientPathMigrationTest.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/ClientPathMigrationTest.kt index 160bc0b77..d52dc6ac8 100644 --- a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/ClientPathMigrationTest.kt +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/ClientPathMigrationTest.kt @@ -150,13 +150,21 @@ class ClientPathMigrationTest { drainOutbound(client, nowMillis = 0L) // 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) assertEquals(PathMigrationResult.Started, triggered) 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 - // PATH_CHALLENGE. + // PATH_CHALLENGE stamped with the NEW DCID. val challengeOut = drainOutbound(client, nowMillis = 100L) assertTrue(challengeOut != null, "client must emit a packet carrying PATH_CHALLENGE") val outboundFrames = pipe.decryptClientApplicationFrames(challengeOut) @@ -171,9 +179,11 @@ class ClientPathMigrationTest { )!! feedDatagram(client, response, nowMillis = 200L) - // Step 5 — DCID must have rotated to the new bytes; the prior - // sequence (0) must be queued for RETIRE_CONNECTION_ID. - assertContentEquals(newCidBytes, client.destinationConnectionId.bytes, "DCID must rotate to new bytes") + // Step 5 — validation succeeded; DCID is still the new bytes + // (already rotated at step 2), the prior sequence (0) is + // queued for RETIRE_CONNECTION_ID, and activeCidSequence now + // reflects the new sequence. + assertContentEquals(newCidBytes, client.destinationConnectionId.bytes) assertEquals(1L, client.pathValidator.activeCidSequence) assertTrue( client.pathValidator.pendingRetireSequences.contains(0L), @@ -197,36 +207,43 @@ class ClientPathMigrationTest { } @Test - fun pathResponseWithMismatchingPayloadDoesNotRotateDcid() = + fun pathResponseWithMismatchingPayloadKeepsValidatingAndDcid() = runBlocking { 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 = pipe.buildServerApplicationDatagram( listOf( NewConnectionIdFrame( sequenceNumber = 1L, retirePriorTo = 0L, - connectionId = ByteArray(8) { 0x01 }, + connectionId = newCid, statelessResetToken = ByteArray(16) { 0x10 }, ), ), )!! feedDatagram(client, offer, nowMillis = 0L) 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 - // 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 = pipe.buildServerApplicationDatagram( listOf(PathResponseFrame(ByteArray(8) { 0xFF.toByte() })), )!! 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") + assertEquals(0L, client.pathValidator.activeCidSequence, "active sequence still old until validation succeeds") } @Test @@ -257,4 +274,80 @@ class ClientPathMigrationTest { "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) + } } diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/PathValidatorTest.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/PathValidatorTest.kt index d379b26e8..0a5f81433 100644 --- a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/PathValidatorTest.kt +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/PathValidatorTest.kt @@ -166,7 +166,7 @@ class PathValidatorTest { assertTrue(outcome is PathValidator.ValidationOutcome.Validated, "got $outcome") assertEquals(1L, outcome.newSequence) assertEquals(0L, outcome.retiredSequence) - assertContentEquals(ByteArray(8) { 0xAA.toByte() }, outcome.newConnectionIdBytes) + assertContentEquals(ByteArray(8) { 0xAA.toByte() }, outcome.connectionId) assertEquals(1L, v.activeCidSequence) assertTrue(v.state is PathValidationState.Succeeded) assertTrue(v.pendingRetireSequences.contains(0L), "old sequence must be queued for RETIRE_CONNECTION_ID") @@ -191,7 +191,12 @@ class PathValidatorTest { } @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 } }) v.recordPeerNewConnectionId(1L, 0L, ByteArray(8) { 0x01 }, ByteArray(16)) val pto = 1_000L @@ -203,6 +208,10 @@ class PathValidatorTest { assertEquals(1L, abandoned.newCidSequence) 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}", + ) } @Test