test(quic): exercise the actual driver PTO path, not a simulation
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 commitsc0d7b6031(qlog merge) and again incf2303a38(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
This commit is contained in:
+48
-39
@@ -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
|
||||
|
||||
+10
-9
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user