nick/amethyst
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fix(quic): consecutivePtoCount must advance once per PTO firing

Browse Source 
RFC 9002 §6.2.2 says the consecutive-PTO count goes up by exactly
ONE per PTO timer expiry, so the §6.2.1 backoff is `pto_base * 2^count`
per firing. Pre-fix, the count incremented THREE times per firing:

  1. inside `handlePtoFired` itself,
  2. inside `requeueInflightForProbe` (which `handlePtoFired` calls),
  3. again from the send loop's between-probe re-requeue (RFC §6.2.4
     two-packet probe budget calls `requeueInflightForProbe` a
     second time).

That made the effective backoff `2^(3*N)` per firing — 1×, 8×, 64×
of pto_base. With pto_base ≈ 150 ms post-handshake, PTO #3 didn't
fire until ~11 s post-handshake, well past the 5 s amp-limited
idle timeout strict servers (quic-go) enforce.

Quic-go's `amplificationlimit` testcase exposed this. The sim
drops client packets 2–7. Our PTO #2 second probe is packet #8
— the first one that gets through. With correct 1× increment
per PTO, packet #8 reaches the server in ~900 ms; pre-fix it
arrived at ~12 s, after quic-go had already destroyed the
connection ("Amplification window limited" → "Destroying
connection: timeout: no recent network activity"). Same root
cause for `handshakeloss` flakiness against quic-go (multiconnect
under 30% loss can't recover within the 30 s per-iteration
budget when PTO ramps to 64× by iteration 3).

Fix:
  - Move the count increment to the top of `handlePtoFired`,
    before it calls `requeueInflightForProbe`. The threshold
    check inside the latter (RFC 9000 §9 — trigger
    PATH_PROBE_PTO_THRESHOLD migration) reads the
    post-increment value, preserving the prior 2-PTO-firing
    trigger semantics.
  - Remove the count increment from `requeueInflightForProbe`.
    The send loop's between-probe re-requeue stays a no-op for
    the count (same PTO firing).

Tests: PtoCryptoRetransmitTest.consecutivePtoCountAdvancesByOnePerPtoFiringNotPerProbePacket
pins the contract (full handlePtoFired → drain → re-requeue →
drain → handlePtoFired sequence; asserts count after each step).

Verified end-to-end against the live interop runner:
  - amplificationlimit vs quic-go: 5/5 (was 0/3 — consistent
    30 s timeout). Now passes in ~7 s.
  - handshakeloss vs quic-go: 5/5 (was 2/3 — flaky multiconnect
    iteration timeouts). Now consistently ~30 s.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Vitor Pamplona
2026-05-09 10:26:54 -04:00
parent e8991fab69
commit 3a5c010993
2 changed files with 86 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files
quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionDriver.kt
+25 -12
View File
@@ -508,12 +508,23 @@ class QuicConnectionDriver(
* (or the fresh one — both are valid) and is at worst a no-op.
*/
internal suspend fun handlePtoFired(conn: QuicConnection) {
// RFC 9002 §6.2.2: the consecutive-PTO count advances by exactly
// ONE per PTO timer expiry, so the §6.2.1 backoff is
// `pto_base * 2^count` per firing. Pre-2026-05-08 we incremented
// THREE times per firing — here + inside [requeueInflightForProbe]
// + again from the send loop's between-probe re-requeue (RFC
// §6.2.4 two-packet probe budget) — turning the backoff into
// `2^(3*N)` — 1×, 8×, 64× per PTO instead of 1×, 2×, 4×.
// Quic-go's `amplificationlimit` testcase exposed this: after the
// sim drops client packets 2-7, we couldn't get a probe to the
// server within its 5 s amp-limited idle timeout because PTO #3
// didn't fire until ~11 s post-handshake.
//
// Increment FIRST so [requeueInflightForProbe]'s threshold check
// sees the post-increment value. The increment must happen exactly
// once per PTO event — not per call to [requeueInflightForProbe],
// because the send loop calls that helper again between the first
// and second probe (RFC 9002 §6.2.4) and that re-requeue is part
// of the SAME PTO event.
// (RFC 9000 §9 — trigger migration after PATH_PROBE_PTO_THRESHOLD
// consecutive PTOs) sees the post-increment value, matching the
// constant's natural reading: "after N consecutive PTOs with no
// progress, trigger migration on the Nth PTO firing".
conn.consecutivePtoCount = (conn.consecutivePtoCount + 1).coerceAtMost(6)
conn.pendingPing = true
requeueInflightForProbe(conn)
@@ -550,13 +561,15 @@ internal suspend fun requeueInflightForProbe(conn: QuicConnection) {
if (conn.initial.sendProtection != null && !conn.initial.keysDiscarded) {
conn.requeueAllInflightCrypto(EncryptionLevel.INITIAL)
}
// The PTO count is incremented in [handlePtoFired] BEFORE this
// helper runs, so the threshold check below sees the post-increment
// value (matching the constant's natural reading: "after N
// consecutive PTOs with no progress, trigger migration on the Nth
// PTO firing"). The send loop's between-probe re-requeue calls
// this helper without bumping the count again — that's the same
// PTO event, not a new one.
// [conn.consecutivePtoCount] is incremented exactly once per PTO
// firing inside [handlePtoFired]; the path-validation threshold
// check below reads the already-incremented value. Pre-2026-05-08
// this function ALSO incremented the count, and the send loop's
// between-probe re-requeue called this function again — turning
// one PTO into +3 to the count. The amplificationlimit interop
// testcase against quic-go flushed this: PTO #3 didn't fire until
// ~11 s post-handshake, well past the 5 s amp-limited idle
// timeout. See [handlePtoFired] kdoc for the full mechanism.
// 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
quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/PtoCryptoRetransmitTest.kt
+61
View File
@@ -281,6 +281,67 @@ class PtoCryptoRetransmitTest {
)
}
@Test
fun consecutivePtoCountAdvancesByOnePerPtoFiringNotPerProbePacket() =
runBlocking {
// RFC 9002 §6.2.2: the consecutive-PTO count advances by
// exactly ONE per PTO timer expiry (so the §6.2.1 backoff
// is `pto_base * 2^count` per firing). Pre-2026-05-08 the
// count incremented THREE times per firing — once in
// `handlePtoFired`, once in `requeueInflightForProbe`,
// and again from the send loop's between-probe re-requeue
// — making the effective backoff `2^(3*N)` (1×, 8×, 64×
// per PTO). Quic-go's `amplificationlimit` testcase
// exposed this: PTO #3 didn't fire until ~11 s
// post-handshake, well past quic-go's 5 s amp-limited
// idle timeout. See the explicit kdoc in
// [com.vitorpamplona.quic.connection.handlePtoFired].
//
// This test drives the EXACT helpers the production send
// loop calls (handlePtoFired + the between-probe
// requeueInflightForProbe) and asserts the count grows
// by 1 per PTO firing, not per probe packet.
val client = newClientWithStartedHandshake()
drainOutbound(client, nowMillis = 1L)
assertEquals(0, client.consecutivePtoCount, "fresh client starts at 0")
// PTO #1 — full simulation including budget consumption.
handlePtoFired(client)
assertEquals(
1,
client.consecutivePtoCount,
"handlePtoFired alone must take count from 0 to 1",
)
// First probe drained.
drainOutbound(client, nowMillis = 2L)
client.pendingProbePackets--
// Send loop's between-probe re-requeue. Pre-fix this
// bumped the count to 2 inside the same PTO firing.
requeueInflightForProbe(client)
assertEquals(
1,
client.consecutivePtoCount,
"between-probe re-requeue must NOT bump the count — same PTO firing",
)
// Second probe drained.
drainOutbound(client, nowMillis = 3L)
client.pendingProbePackets--
// PTO #2 — count must advance to 2, not 4.
handlePtoFired(client)
assertEquals(2, client.consecutivePtoCount, "PTO #2 takes count from 1 to 2")
drainOutbound(client, nowMillis = 4L)
client.pendingProbePackets--
requeueInflightForProbe(client)
assertEquals(2, client.consecutivePtoCount)
drainOutbound(client, nowMillis = 5L)
client.pendingProbePackets--
// PTO #3 — count → 3, not 6 (capped).
handlePtoFired(client)
assertEquals(3, client.consecutivePtoCount, "PTO #3 takes count from 2 to 3")
}
/**
* Decode an Initial-level long-header packet from a freshly-drained
* datagram and return its frames. We open with the client's own