From d5c854befa62e738e10998ae6122a5dee72d0dab Mon Sep 17 00:00:00 2001 From: Vitor Pamplona Date: Thu, 7 May 2026 15:51:51 -0400 Subject: [PATCH] fix(quic): PTO retransmits handshake CRYPTO + STREAM data on stalled-ACK paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../quic/connection/QuicConnection.kt | 43 +++++++++++ .../quic/connection/QuicConnectionDriver.kt | 72 +++++++++++-------- 2 files changed, 85 insertions(+), 30 deletions(-) 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 e23cabdaf..c572d9f11 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt @@ -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 = streams 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 f0be55c39..903cc00ef 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionDriver.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionDriver.kt @@ -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 - }