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 171533a62..c3d3ade09 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt @@ -769,8 +769,16 @@ class QuicConnection( internal fun onTokensAcked(tokens: List) { for (token in tokens) { when (token) { - com.vitorpamplona.quic.connection.recovery.RecoveryToken.Ack -> { - // ACK-of-ACK is a no-op. + is com.vitorpamplona.quic.connection.recovery.RecoveryToken.Ack -> { + // ACK-of-ACK: peer has now received our ACK, so + // we can drop everything at-or-below + // [token.largestAcked] from the inbound tracker. + // RFC 9000 §13.2.1 doesn't require us to keep + // re-advertising acknowledged PNs once the peer + // has confirmed receipt of our ACK that covered + // them. Without this the tracker's range list + // grows over the connection lifetime. + levelState(token.level).ackTracker.purgeBelow(token.largestAcked + 1) } is com.vitorpamplona.quic.connection.recovery.RecoveryToken.MaxStreamsUni, @@ -837,7 +845,7 @@ class QuicConnection( internal fun onTokensLost(tokens: List) { for (token in tokens) { when (token) { - com.vitorpamplona.quic.connection.recovery.RecoveryToken.Ack -> { + is com.vitorpamplona.quic.connection.recovery.RecoveryToken.Ack -> { // ACK frames are not retransmittable per RFC 9000 // §13.2.1; the peer's own ACKs cover newer ranges. } diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionParser.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionParser.kt index 829513344..03e825754 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionParser.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionParser.kt @@ -164,12 +164,18 @@ private fun dispatchFrames( for (frame in frames) { when (frame) { is AckFrame -> { - // Purge our own ACK tracker below the peer's largest - // acknowledged: the peer has confirmed receipt of those - // ACKs, so we don't need to keep advertising them — - // without this the range list grows unboundedly on long - // connections. - state.ackTracker.purgeBelow(frame.largestAcknowledged - frame.firstAckRange) + // The peer's `frame.largestAcknowledged` is in OUR + // outbound PN space — the inbound `state.ackTracker` + // tracks PNs WE RECEIVED from the peer. The previous + // purge here was conceptually wrong (mixed two + // unrelated number spaces) but happened to mostly + // work because both PN spaces grow at similar rates. + // The correct purge is now driven by the + // ACK-of-ACK dispatch in [QuicConnection.onTokensAcked] + // for [RecoveryToken.Ack]: when the peer ACKs the + // packet that carried our outbound ACK frame, we + // know the peer received our ACK and we can drop + // the corresponding inbound PNs from the tracker. // Step 5 of `quic/plans/2026-05-04-control-frame-retransmit.md`: // (a) snapshot the send time of the largest-acked PN // BEFORE drain so we can update RTT, (b) drain ACK'd diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionWriter.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionWriter.kt index 86560fb56..eaf711ff3 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionWriter.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionWriter.kt @@ -248,7 +248,7 @@ private fun collectHandshakeLevelFrames( val tokens = mutableListOf() state.ackTracker.buildAckFrame(nowMillis, conn.config.ackDelayExponent.toInt())?.let { frames += it - tokens += RecoveryToken.Ack + tokens += RecoveryToken.Ack(level = level, largestAcked = it.largestAcknowledged) } val cryptoChunk = state.cryptoSend.takeChunk(maxBytes = 1100) if (cryptoChunk != null && cryptoChunk.data.isNotEmpty()) { @@ -351,7 +351,7 @@ private fun buildApplicationPacket( state.ackTracker.buildAckFrame(nowMillis, conn.config.ackDelayExponent.toInt())?.let { frames += it - tokens += RecoveryToken.Ack + tokens += RecoveryToken.Ack(level = EncryptionLevel.APPLICATION, largestAcked = it.largestAcknowledged) } // Step 7: PTO probe. The driver sets `pendingPing` when its diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/recovery/RecoveryToken.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/recovery/RecoveryToken.kt index 65125fac3..8ed60acff 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/recovery/RecoveryToken.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/recovery/RecoveryToken.kt @@ -53,12 +53,33 @@ package com.vitorpamplona.quic.connection.recovery */ sealed class RecoveryToken { /** - * Marks an ACK frame in the carrying packet. Recorded so the - * sent-packet map's invariant ("every retained packet has a - * non-empty `tokens` list") holds for ACK-only packets too, but - * never re-emitted on loss. + * Marks an ACK frame in the carrying packet. Carries enough + * context for the dispatcher to purge our own [com.vitorpamplona.quic.recovery.AckTracker] + * once the peer ACKs the carrying packet — i.e. an ACK-of-ACK. + * + * RFC 9000 §13.2.1: ACK frames are not ack-eliciting and not + * retransmitted. But the ACK we sent IS itself acknowledgeable + * (it rides on a packet whose other frames may be ack-eliciting), + * and the peer's ACK of THAT packet means the peer has now + * received our ACK for everything up to [largestAcked]. We can + * stop including those PNs in subsequent outbound ACK frames. + * + * Pre-fix the parser purged on every inbound ACK using the + * peer's `largestAcknowledged - firstAckRange` value — but that + * is in OUR outbound PN space, not the inbound PN space the + * tracker holds. The values happened to grow at similar rates so + * the bug rarely manifested as outright wrongness, just + * range-list bloat over long sessions where the two spaces drift + * apart (e.g. listener receiving ~50 audio frames/sec while + * sending ~1 ACK/sec). + * + * [level] routes the purge to the right per-space [com.vitorpamplona.quic.connection.LevelState.ackTracker] + * since we track separately for Initial / Handshake / Application. */ - data object Ack : RecoveryToken() + data class Ack( + val level: com.vitorpamplona.quic.connection.EncryptionLevel, + val largestAcked: Long, + ) : RecoveryToken() /** * `MAX_STREAMS_UNI` extension we sent. On loss, only re-emit if diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/AckTrackerPurgeOnAckOfAckTest.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/AckTrackerPurgeOnAckOfAckTest.kt new file mode 100644 index 000000000..37e304ed0 --- /dev/null +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/AckTrackerPurgeOnAckOfAckTest.kt @@ -0,0 +1,166 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.quic.connection + +import com.vitorpamplona.quic.connection.recovery.RecoveryToken +import kotlinx.coroutines.runBlocking +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** + * RFC 9000 §13.2.1: an ACK frame is itself acknowledged via the + * carrying packet. Once the peer ACKs that packet, we know the peer + * received our ACK and we can drop the corresponding inbound PNs from + * our [com.vitorpamplona.quic.recovery.AckTracker]. + * + * Pre-fix, `QuicConnectionParser` purged the tracker on every inbound + * ACK using `frame.largestAcknowledged - frame.firstAckRange` — but + * that value is in OUR outbound PN space, not the inbound PN space the + * tracker holds. The mistake mostly hid because both spaces grow at + * similar rates, but caused range-list bloat over long sessions where + * traffic is asymmetric (e.g. listener receives ~50 audio frames/sec + * while sending ~1 ACK/sec back). + */ +class AckTrackerPurgeOnAckOfAckTest { + private fun newConn(): QuicConnection = + QuicConnection( + serverName = "example.test", + config = QuicConnectionConfig(), + tlsCertificateValidator = + com.vitorpamplona.quic.tls + .PermissiveCertificateValidator(), + ) + + @Test + fun ackOfAck_purgesTrackerBelowLargestAcked() = + runBlocking { + val conn = newConn() + // Simulate having received 5 inbound packets from the peer. + for (pn in 0L..4L) { + conn.application.ackTracker.receivedPacket(pn, ackEliciting = true, receivedAtMillis = 1L) + } + assertEquals(4L, conn.application.ackTracker.largestReceived()) + assertFalse(conn.application.ackTracker.isEmpty()) + + // Peer ACKs the packet that carried our outbound ACK + // covering up to PN 4. + conn.lock.lock() + try { + conn.onTokensAcked( + listOf( + RecoveryToken.Ack( + level = EncryptionLevel.APPLICATION, + largestAcked = 4L, + ), + ), + ) + } finally { + conn.lock.unlock() + } + // Tracker is now empty: peer has confirmed receipt of our + // ACK that covered everything up to PN 4. Re-advertising + // those PNs in subsequent outbound ACKs is wasted bytes. + assertTrue(conn.application.ackTracker.isEmpty(), "tracker fully purged below largestAcked + 1") + } + + @Test + fun ackOfAck_routesToCorrectLevel() = + runBlocking { + val conn = newConn() + // Independent state on Initial vs Application trackers. + for (pn in 0L..2L) { + conn.initial.ackTracker.receivedPacket(pn, ackEliciting = true, receivedAtMillis = 1L) + } + for (pn in 0L..4L) { + conn.application.ackTracker.receivedPacket(pn, ackEliciting = true, receivedAtMillis = 1L) + } + + // Peer ACKs our Initial-level outbound ACK. + conn.lock.lock() + try { + conn.onTokensAcked( + listOf( + RecoveryToken.Ack(level = EncryptionLevel.INITIAL, largestAcked = 2L), + ), + ) + } finally { + conn.lock.unlock() + } + // Initial tracker drained; Application tracker untouched. + assertTrue(conn.initial.ackTracker.isEmpty()) + assertEquals(4L, conn.application.ackTracker.largestReceived()) + } + + @Test + fun ackOfAck_partialPurge_keepsHigherPns() = + runBlocking { + val conn = newConn() + for (pn in 0L..9L) { + conn.application.ackTracker.receivedPacket(pn, ackEliciting = true, receivedAtMillis = 1L) + } + // Peer ACKs our outbound ACK that covered up to PN 4 only; + // the tracker's higher-PN ranges (5..9) must survive. + conn.lock.lock() + try { + conn.onTokensAcked( + listOf( + RecoveryToken.Ack(level = EncryptionLevel.APPLICATION, largestAcked = 4L), + ), + ) + } finally { + conn.lock.unlock() + } + assertFalse(conn.application.ackTracker.isEmpty()) + assertEquals(9L, conn.application.ackTracker.largestReceived()) + } + + @Test + fun ackOfAck_outOfOrder_isSafe() = + runBlocking { + // Two outbound ACKs go out: ACK#A covers up-to PN 4, ACK#B + // covers up-to PN 9. Peer ACKs ACK#B first, then ACK#A. + // After ACK#B drains, tracker is empty. ACK#A's purge is + // a no-op — must not throw, must not re-resurrect anything. + val conn = newConn() + for (pn in 0L..9L) { + conn.application.ackTracker.receivedPacket(pn, ackEliciting = true, receivedAtMillis = 1L) + } + conn.lock.lock() + try { + conn.onTokensAcked( + listOf( + RecoveryToken.Ack(level = EncryptionLevel.APPLICATION, largestAcked = 9L), + ), + ) + assertTrue(conn.application.ackTracker.isEmpty()) + conn.onTokensAcked( + listOf( + RecoveryToken.Ack(level = EncryptionLevel.APPLICATION, largestAcked = 4L), + ), + ) + assertTrue(conn.application.ackTracker.isEmpty()) + } finally { + conn.lock.unlock() + } + } +} diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/OnTokensLostTest.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/OnTokensLostTest.kt index f4978ff1b..c14da3ff4 100644 --- a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/OnTokensLostTest.kt +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/OnTokensLostTest.kt @@ -49,7 +49,7 @@ class OnTokensLostTest { val conn = newConn() conn.lock.lock() try { - conn.onTokensLost(listOf(RecoveryToken.Ack)) + conn.onTokensLost(listOf(RecoveryToken.Ack(level = EncryptionLevel.APPLICATION, largestAcked = 0L))) } finally { conn.lock.unlock() } @@ -161,7 +161,7 @@ class OnTokensLostTest { conn.advertisedMaxData = 5_000_000L conn.onTokensLost( listOf( - RecoveryToken.Ack, + RecoveryToken.Ack(level = EncryptionLevel.APPLICATION, largestAcked = 0L), RecoveryToken.MaxStreamsUni(maxStreams = 150L), RecoveryToken.MaxStreamsBidi(maxStreams = 200L), RecoveryToken.MaxData(maxData = 5_000_000L), diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/RetransmitIntegrationTest.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/RetransmitIntegrationTest.kt index 3eb1cf884..73a05ce1c 100644 --- a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/RetransmitIntegrationTest.kt +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/RetransmitIntegrationTest.kt @@ -80,7 +80,10 @@ class RetransmitIntegrationTest { sentAtMillis = 1L, ackEliciting = true, sizeBytes = 64, - tokens = listOf(RecoveryToken.Ack), + tokens = + listOf( + RecoveryToken.Ack(level = EncryptionLevel.APPLICATION, largestAcked = 0L), + ), ) // Drain the ACK'd packet from the map (simulating the // parser path). diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/recovery/RecoveryTokenTest.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/recovery/RecoveryTokenTest.kt index 5b952dd56..1f9e5f5a8 100644 --- a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/recovery/RecoveryTokenTest.kt +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/recovery/RecoveryTokenTest.kt @@ -23,12 +23,11 @@ package com.vitorpamplona.quic.connection.recovery import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertNotEquals -import kotlin.test.assertTrue /** * Type-level invariants for [RecoveryToken]: equality + hashCode - * correctness across data-class variants, the singleton [Ack] object, - * and the typed-token shape required by the dispatcher in step 6 of + * correctness across data-class variants and the typed-token shape + * required by the dispatcher in step 6 of * `quic/plans/2026-05-04-control-frame-retransmit.md`. * * No connection state is exercised here — these are unit tests on @@ -36,11 +35,18 @@ import kotlin.test.assertTrue */ class RecoveryTokenTest { @Test - fun ack_isSingleton() { - val a: RecoveryToken = RecoveryToken.Ack - val b: RecoveryToken = RecoveryToken.Ack - // `data object Ack` ⇒ same reference, same hash. - assertTrue(a === b) + fun ack_carriesLevelAndLargestAcked() { + val a: RecoveryToken = + RecoveryToken.Ack( + level = com.vitorpamplona.quic.connection.EncryptionLevel.APPLICATION, + largestAcked = 42L, + ) + val b: RecoveryToken = + RecoveryToken.Ack( + level = com.vitorpamplona.quic.connection.EncryptionLevel.APPLICATION, + largestAcked = 42L, + ) + // Data class equality, not identity. assertEquals(a, b) assertEquals(a.hashCode(), b.hashCode()) } @@ -93,7 +99,10 @@ class RecoveryTokenTest { // compiling — caught at build time, not at runtime. val tokens: List = listOf( - RecoveryToken.Ack, + RecoveryToken.Ack( + level = com.vitorpamplona.quic.connection.EncryptionLevel.APPLICATION, + largestAcked = 0L, + ), RecoveryToken.MaxStreamsUni(150L), RecoveryToken.MaxStreamsBidi(150L), RecoveryToken.MaxData(1_000_000L), @@ -116,7 +125,7 @@ class RecoveryTokenTest { val labels = tokens.map { when (it) { - RecoveryToken.Ack -> "ack" + is RecoveryToken.Ack -> "ack" is RecoveryToken.MaxStreamsUni -> "msu:${it.maxStreams}" is RecoveryToken.MaxStreamsBidi -> "msb:${it.maxStreams}" is RecoveryToken.MaxData -> "md:${it.maxData}" diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/recovery/SentPacketTest.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/recovery/SentPacketTest.kt index 0f1a2ea91..ac10b30f6 100644 --- a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/recovery/SentPacketTest.kt +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/recovery/SentPacketTest.kt @@ -20,6 +20,7 @@ */ package com.vitorpamplona.quic.connection.recovery +import com.vitorpamplona.quic.connection.EncryptionLevel import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertNotEquals @@ -63,7 +64,7 @@ class SentPacketTest { sentAtMillis = 0L, ackEliciting = false, sizeBytes = 16, - tokens = listOf(RecoveryToken.Ack), + tokens = listOf(RecoveryToken.Ack(level = EncryptionLevel.APPLICATION, largestAcked = 0L)), ) val differentPn = a.copy(packetNumber = 8L) assertNotEquals(a, differentPn) @@ -95,10 +96,13 @@ class SentPacketTest { sentAtMillis = 0L, ackEliciting = false, sizeBytes = 24, - tokens = listOf(RecoveryToken.Ack), + tokens = listOf(RecoveryToken.Ack(level = EncryptionLevel.APPLICATION, largestAcked = 0L)), ) assertTrue(ackOnly.tokens.isNotEmpty()) - assertEquals(RecoveryToken.Ack, ackOnly.tokens.single()) + assertEquals( + RecoveryToken.Ack(level = EncryptionLevel.APPLICATION, largestAcked = 0L), + ackOnly.tokens.single(), + ) } @Test