fix(quic): correct AckTracker.purgeBelow via ACK-of-ACK dispatch

Pre-fix, QuicConnectionParser purged the inbound AckTracker on every inbound AckFrame using `frame.largestAcknowledged - frame.firstAckRange` — but that value lives in OUR outbound PN space, while the tracker holds inbound PNs we received from the peer. The two PN spaces are unrelated; the bug mostly hid because they 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 back ~1 ACK/sec). The correct semantics: purge only when the peer has confirmed receipt of OUR outbound ACK frame. Now driven by the ACK-of-ACK dispatch. - RecoveryToken.Ack changed from data object to data class carrying (level, largestAcked) — the encryption level and the largest inbound PN our outbound ACK frame covered. - QuicConnectionWriter populates these fields from the AckFrame at emit time. - QuicConnection.onTokensAcked dispatches RecoveryToken.Ack to levelState(level).ackTracker.purgeBelow(largestAcked + 1). - The wrong purge in QuicConnectionParser is removed (replaced with a comment pointing at the new dispatch path). Listed in the audit-summary deferred-work as item 6 (`AckTracker.purgeBelow threshold semantics`). New test: AckTrackerPurgeOnAckOfAckTest (4 cases — purge on ack-of-ack, level routing, partial purge keeps higher PNs, out-of-order ACKs are safe). https://claude.ai/code/session_01PYYez8a6sjiakyjAxsfCEQ
2026-05-05 01:30:02 +00:00
parent 70af1953dc
commit e3a3ffd1d9
9 changed files with 249 additions and 32 deletions
@@ -769,8 +769,16 @@ class QuicConnection(
    internal fun onTokensAcked(tokens: List<com.vitorpamplona.quic.connection.recovery.RecoveryToken>) {
        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<com.vitorpamplona.quic.connection.recovery.RecoveryToken>) {
        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.
                }
@@ -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
@@ -248,7 +248,7 @@ private fun collectHandshakeLevelFrames(
    val tokens = mutableListOf<RecoveryToken>()
    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
@@ -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
@@ -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()
+            }
+        }
+}
@@ -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),
@@ -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).
@@ -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<RecoveryToken> =
            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}"
@@ -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