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 57fdc88e7..279e38dd9 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/PathValidator.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/PathValidator.kt @@ -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() } /** 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 985e1a06f..af9b080de 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt @@ -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 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 0b7c2fc05..257c80aa7 100644 --- a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/ClientPathMigrationTest.kt +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/ClientPathMigrationTest.kt @@ -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() + 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() + 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 { 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 545fcf39e..4944352eb 100644 --- a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/PathValidatorTest.kt +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/PathValidatorTest.kt @@ -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