From b579c766a4c8f101aa7c315a7b5138cb89af8c23 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 7 May 2026 02:50:31 +0000 Subject: [PATCH] test(quic): exercise the actual driver PTO path, not a simulation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing PtoCryptoRetransmitTest simulated the driver inline: it set pendingPing=true and called requeueAllInflightCrypto by hand, then asserted the next drain emitted CRYPTO. That checked the helpers worked but never noticed when the DRIVER stopped calling requeueAllInflightCrypto — which is exactly the regression that bit us in commits c0d7b6031 (qlog merge) and again in cf2303a38 (lock-split refactor). Extract the PTO-fired logic from QuicConnectionDriver.sendLoop into a top-level internal helper handlePtoFired(conn). Driver and test now both call the same function — if anyone unwires the requeue from that helper or rewrites sendLoop to do volatile-only updates, the test breaks. Verified the regression-catch by stripping the requeue from handlePtoFired and re-running the test: it fails with an AssertionError on the PTO retransmit packet check, as expected. Restored the helper, test green again. https://claude.ai/code/session_01HcvfQq1ttPV9PkRoJb4nyT --- .../quic/connection/QuicConnectionDriver.kt | 87 ++++++++++--------- .../connection/PtoCryptoRetransmitTest.kt | 19 ++-- 2 files changed, 58 insertions(+), 48 deletions(-) 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 23c1f792f..d20e62781 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionDriver.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionDriver.kt @@ -154,45 +154,7 @@ class QuicConnectionDriver( Unit } if (woke == null) { - // PTO fired. RFC 9002 §6.2.4: the probe packet MUST - // be ack-eliciting at the encryption level with - // unacknowledged data, and SHOULD retransmit lost - // data rather than just emit a PING. - // - // Pre-1-RTT we have a concrete thing to retransmit: - // the unacknowledged ClientHello (at Initial) or - // ClientFinished (at Handshake). Requeue ALL - // currently-inflight CRYPTO bytes for the highest - // active pre-application level so the next drain's - // takeChunk emits a fresh CRYPTO frame at the - // original offset. pendingPing stays set as a - // fallback only — `collectHandshakeLevelFrames` - // skips the PING when CRYPTO is in the same frame - // list. aioquic strictly rejects pre-handshake - // Initials with no CRYPTO frame ("Packet contains - // no CRYPTO frame"), so a bare-PING probe is fatal. - // - // Post-handshake (1-RTT installed) we keep the - // bare-PING behavior; STREAM retransmit is driven - // by ACK-driven packet-threshold loss detection. - // - // pendingPing + consecutivePtoCount are @Volatile. - // requeueAllInflightCrypto mutates the level's - // cryptoSend buffer; the writer reads it under - // levelLock during drainOutbound, so we take that - // lock for the requeue to avoid racing the writer's - // takeChunk with our requeue mid-build. - connection.pendingPing = true - if (connection.application.sendProtection == null) { - val level = highestPreApplicationLevel(connection) - if (level != null) { - connection.levelState(level).levelLock.withLock { - connection.requeueAllInflightCrypto(level) - } - } - } - connection.consecutivePtoCount = - (connection.consecutivePtoCount + 1).coerceAtMost(6) + handlePtoFired(connection) } } } @@ -263,6 +225,53 @@ 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. + * + * `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. + * + * Why aioquic interop demands this: aioquic strictly rejects pre- + * handshake Initials that contain no CRYPTO frame + * (`CONNECTION_CLOSE 0x0 "Packet contains no CRYPTO frame"`). A + * bare-PING probe before the ClientHello is acknowledged is fatal. + * + * Extracted from [QuicConnectionDriver.sendLoop]'s PTO branch into a + * top-level helper so the unit test in + * [com.vitorpamplona.quic.connection.PtoCryptoRetransmitTest] + * can invoke the EXACT logic the live driver does, without standing + * up a UDP socket. Earlier shapes simulated the steps inline in the + * test, which let the driver-side wiring regress twice + * (commits c0d7b6031, then again in the lock-split refactor) without + * any test breaking. + * + * `pendingPing` and `consecutivePtoCount` are `@Volatile` so we set + * them outside any external lock. `requeueAllInflightCrypto` mutates + * the level's `cryptoSend` buffer; we take `levelLock` for the + * requeue so the writer's `takeChunk` can't observe a half-mutated + * inflight queue mid-build. + */ +internal suspend fun handlePtoFired(conn: QuicConnection) { + conn.pendingPing = true + if (conn.application.sendProtection == null) { + val level = highestPreApplicationLevel(conn) + if (level != null) { + conn.levelState(level).levelLock.withLock { + conn.requeueAllInflightCrypto(level) + } + } + } + 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 diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/PtoCryptoRetransmitTest.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/PtoCryptoRetransmitTest.kt index 60f39234a..ad610c1e3 100644 --- a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/PtoCryptoRetransmitTest.kt +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/PtoCryptoRetransmitTest.kt @@ -91,15 +91,16 @@ class PtoCryptoRetransmitTest { "no PTO yet, no fresh data → second drain must be empty (got ${emptyDrain?.size} bytes)", ) - // Simulate the driver's PTO branch firing: requeue the - // unacknowledged CRYPTO and set the PING flag. - client.lock.lock() - try { - client.requeueAllInflightCrypto(EncryptionLevel.INITIAL) - client.pendingPing = true - } finally { - client.lock.unlock() - } + // Drive the EXACT helper QuicConnectionDriver.sendLoop + // calls when its PTO timer fires. Earlier versions of + // this test inlined the simulation (set pendingPing, + // call requeueAllInflightCrypto manually) — but that + // hid the regression where the driver itself stopped + // calling requeueAllInflightCrypto. Calling + // [handlePtoFired] keeps the test in lockstep with the + // production code path: if anyone unwires the requeue + // again, this test breaks. + handlePtoFired(client) // Next drain must emit a fresh Initial packet carrying // the ClientHello CRYPTO at the original offset.