fix(quic): PTO retransmits handshake CRYPTO + STREAM data on stalled-ACK paths

RFC 9002 §6.2.4 says a PTO probe SHOULD retransmit unacked data, not emit a bare PING. Two gaps in our handler surfaced via interop: 1. Handshake CRYPTO past the 1-RTT-keys-up boundary. The pre-fix handler gated the requeue on `application.sendProtection == null` so once 1-RTT keys were derived, our Finished (still inflight at Handshake level until the peer ACKs it) was never retransmitted. Lost Finished → server never confirms handshake → never sends HANDSHAKE_DONE → connection wedges with ACK-only handshake packets bouncing forever. Surfaced by handshakeloss against aioquic at 30% drop rate (multiconnect iter 12 stuck at t=52s, zero handshake_done). 2. STREAM data when the peer never ACKs anything. Our loss detection gates on `pn < largestAckedPn`, which never advances when every one of our 1-RTT packets is dropped or corrupted en route. Surfaced by handshakecorruption: we send H3 init streams + GET in 1-RTT pn=0, gets corrupted, server never decrypts, never ACKs. Pre-fix the STREAM bytes were never retransmitted; the GET stalled. Fix: handlePtoFired now requeues inflight CRYPTO at every active pre-application level (Initial AND Handshake) regardless of 1-RTT state, and walks streamsList to re-queue inflight STREAM bytes when 1-RTT keys are up. requeueAllInflight is a no-op when nothing is inflight, so calling on already-ACKed / already-discarded levels is harmless. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-07 15:51:51 -04:00
parent 2a4c07ae5e
commit d5c854befa
2 changed files with 85 additions and 30 deletions
@@ -1228,6 +1228,49 @@ class QuicConnection(
        levelState(level).cryptoSend.requeueAllInflight()
    }

+    /**
+     * RFC 9002 §6.2.4 PTO probe — STREAM-data analogue of
+     * [requeueAllInflightCrypto]. Walks every open stream and moves each
+     * stream's sent-but-not-yet-ACK'd byte ranges back to its retransmit
+     * queue, so the next [com.vitorpamplona.quic.connection.drainOutbound]
+     * re-emits the same bytes (at the same offsets, with FIN preserved
+     * per range).
+     *
+     * Why this exists: loss detection ([com.vitorpamplona.quic.connection.recovery.QuicLossDetection.detectAndRemoveLost])
+     * gates on `pn < largestAckedPn`, which means it never fires when
+     * the peer hasn't ACK'd ANYTHING in the application space. That
+     * happens whenever every 1-RTT packet we send is dropped or
+     * corrupted en route — the peer never sees them, never ACKs, and
+     * `largestAckedPn` stays null forever. A bare PING from PTO doesn't
+     * help either: if the PING itself is lost, the peer doesn't see it
+     * either. Re-queuing the data on every PTO ensures that whenever
+     * one of our PROBE packets does land, the peer immediately receives
+     * the application data we'd been trying to send — not an empty PING
+     * that would need a follow-up RTT to retransmit.
+     *
+     * Discovered via `handshakecorruption` against aioquic at 30%
+     * bit-flip rate: client opens HTTP/3 control + QPACK + GET streams
+     * in 1-RTT pn=0, gets corrupted, server never decrypts, never ACKs.
+     * Pre-fix the streams were never retransmitted, the GET stalled,
+     * the multiconnect iteration timed out at 60 s.
+     *
+     * Idempotent: a second consecutive call is a no-op because the first
+     * call drained `inFlight` empty. Best-effort streams (used by
+     * audio-rooms, where Opus tolerates gaps) drop their inflight
+     * ranges instead of re-queueing — see
+     * [com.vitorpamplona.quic.stream.SendBuffer.requeueAllInflight].
+     *
+     * Caller should hold [streamsLock] while iterating; each per-stream
+     * `requeueAllInflight` is internally `synchronized` on its
+     * SendBuffer so the actual byte-range moves are race-free even
+     * under concurrent `takeChunk` from the writer.
+     */
+    internal fun requeueAllInflightStreamData() {
+        for (stream in streamsList) {
+            stream.send.requeueAllInflight()
+        }
+    }
+
    /** Caller must hold [lock]. Snapshot of streams for the driver's send loop. */
    internal fun streamsLocked(): Map<Long, QuicStream> = streams

@@ -227,18 +227,32 @@ class QuicConnectionDriver(
 }

 /**
- * Spec-correct response to a PTO timer firing (RFC 9002 §6.2.4). Pre-1-RTT
- * the probe packet MUST be ack-eliciting at the encryption level with
- * unacknowledged data, and SHOULD retransmit the lost data rather than
- * emit a bare PING — so we requeue ALL inflight CRYPTO bytes at the
- * highest active pre-application level (Initial or Handshake), and the
- * next [drainOutbound] emits a CRYPTO frame at the original offset.
+ * Spec-correct response to a PTO timer firing (RFC 9002 §6.2.4). The probe
+ * packet MUST be ack-eliciting at the encryption level with unacknowledged
+ * data, and SHOULD retransmit the lost data rather than emit a bare PING —
+ * so we requeue inflight CRYPTO bytes at every active pre-application
+ * level (Initial AND Handshake), and the next [drainOutbound] emits a
+ * CRYPTO frame at the original offset. `requeueAllInflight` is a no-op
+ * when nothing is inflight, so calling this for already-ACKed or
+ * already-discarded levels is harmless.
 *
 * `pendingPing` stays set as a fallback. `collectHandshakeLevelFrames`
 * suppresses the PING when CRYPTO is in the same frame list, so we
- * don't waste a frame on top of the retransmit. Post-1-RTT we keep
- * the bare-PING behavior — STREAM loss detection drives retransmit
- * from the ACK that the PING elicits.
+ * don't waste a frame on top of the retransmit.
+ *
+ * Why every active pre-application level, not just the highest: there's a
+ * window between 1-RTT keys becoming installed (server's Finished arrives,
+ * client derives application keys) and the handshake being confirmed
+ * (server's HANDSHAKE_DONE arrives). In that window our own Finished is
+ * still in flight at Handshake level, and the application-space loss
+ * detection that ACK-only PINGs rely on doesn't cover it. If our Finished
+ * is dropped, the server keeps retransmitting handshake CRYPTO forever
+ * trying to elicit our missing ACK-eliciting handshake-level packet,
+ * never confirms the handshake, never sends HANDSHAKE_DONE. Surfaced by
+ * `handshakeloss` against aioquic at 30% drop rate (multiconnect iter 12
+ * stuck at t=52s with zero handshake_done events; pre-fix
+ * `handlePtoFired` gated the requeue on `application.sendProtection ==
+ * null` and skipped Handshake CRYPTO retransmit once 1-RTT keys existed).
 *
 * Why aioquic interop demands this: aioquic strictly rejects pre-
 * handshake Initials that contain no CRYPTO frame
@@ -263,29 +277,27 @@ class QuicConnectionDriver(
 * `requeueAllInflight` operates on the buffer reference we captured
 * (or the fresh one — both are valid) and is at worst a no-op.
 */
-internal fun handlePtoFired(conn: QuicConnection) {
+internal suspend fun handlePtoFired(conn: QuicConnection) {
    conn.pendingPing = true
-    if (conn.application.sendProtection == null) {
-        val level = highestPreApplicationLevel(conn)
-        if (level != null) {
-            conn.requeueAllInflightCrypto(level)
+    if (conn.handshake.sendProtection != null && !conn.handshake.keysDiscarded) {
+        conn.requeueAllInflightCrypto(EncryptionLevel.HANDSHAKE)
+    }
+    if (conn.initial.sendProtection != null && !conn.initial.keysDiscarded) {
+        conn.requeueAllInflightCrypto(EncryptionLevel.INITIAL)
+    }
+    // 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
+    // carrying our HTTP/3 init streams + the GET request) is unrecoverable
+    // because loss detection only runs after the peer ACKs something
+    // and we have nothing else for the peer to ACK. Iterating streamsList
+    // requires streamsLock — `openBidiStream` and friends mutate it under
+    // the same lock, so unlocked iteration races with stream creation.
+    if (conn.application.sendProtection != null) {
+        conn.streamsLock.withLock {
+            conn.requeueAllInflightStreamData()
+            conn.requeueAllInflightCrypto(EncryptionLevel.APPLICATION)
        }
    }
    conn.consecutivePtoCount = (conn.consecutivePtoCount + 1).coerceAtMost(6)
 }
-
-/**
- * Highest encryption level for which `conn` currently holds send keys
- * AND hasn't yet discarded them, given that 1-RTT keys are NOT
- * installed. Returns null when the level state has been completely
- * cleared (e.g. CLOSED after a CONNECTION_CLOSE was sent). Mirrors the
- * private helper in [com.vitorpamplona.quic.connection.QuicConnectionWriter]
- * — kept in lockstep so the driver's PTO branch and the writer's PING
- * placement target the same level.
- */
-private fun highestPreApplicationLevel(conn: QuicConnection): EncryptionLevel? =
-    when {
-        conn.handshake.sendProtection != null -> EncryptionLevel.HANDSHAKE
-        conn.initial.sendProtection != null && !conn.initial.keysDiscarded -> EncryptionLevel.INITIAL
-        else -> null
-    }