Skip to content

Stop requested max ACK delay from growing on each ACK_FREQUENCY exchange - #6340

Open
Joel Kiptoo (Kiptoo-Deus) wants to merge 1 commit into
microsoft:mainfrom
Kiptoo-Deus:fix-ack-frequency-max-ack-delay
Open

Joel Kiptoo (Kiptoo-Deus) wants to merge 1 commit into
microsoft:mainfrom
Kiptoo-Deus:fix-ack-frequency-max-ack-delay

Conversation

@Kiptoo-Deus

Copy link
Copy Markdown

Description

Fixes the first part of #6334: the max ACK delay requested in ACK_FREQUENCY frames grows by one timer tick on every exchange.

The frame's Requested Max Ack Delay was QuicConnGetAckDelay(Connection), which is the configured MaxAckDelayMs plus this endpoint's TimerResolutionMs. The receiver stores the requested value as its own Settings.MaxAckDelayMs (connection.c), and when it later sends ACK_FREQUENCY itself it adds its own timer resolution again. So each exchange adds a tick: 25 → 41 → 57 → 73 ms on Windows, as reported, and 25 → 26 → 27 ms on macOS, where the resolution is 1 ms.

This change requests the configured MaxAckDelayMs instead, raised to the peer's min_ack_delay if it is lower. The receiver rejects requests below its timer resolution, which is what it advertises as min_ack_delay, so the value is still always accepted. ACK_FREQUENCY is only sent when the peer advertised min_ack_delay (QuicConnUpdatePeerPacketTolerance), so PeerTransportParams.MinAckDelay is valid there. The advertised max_ack_delay transport parameter still includes the timer resolution, as before.

A side effect: with delayed ACKs disabled (MaxAckDelayMs == 0), the old code requested 0, which is below any peer's min_ack_delay and which MsQuic itself treats as a protocol violation; it now requests the peer's min_ack_delay.

The second part of the issue (PTO still uses the handshake max_ack_delay rather than the requested value, per draft section 7) is not addressed here. It needs tracking of in-flight and acknowledged ACK_FREQUENCY frames, and after this change the requested value is usually below the handshake value, so PTO stays conservative. I'm happy to follow up on it separately.

Testing

Added QuicTestAckFrequencyMaxAckDelay (Basic/WithFamilyArgs.AckFrequencyMaxAckDelay): a ~52 MB one-way transfer makes the client scheduling-limited so that it sends ACK_FREQUENCY, and the test then checks that MaxAckDelayMs read through QUIC_PARAM_CONN_SETTINGS is still the configured 25 ms on both the client and the server. It fails on main (the server reads 26 on macOS) and passes with this change, for IPv4 and IPv6.

A separate bidirectional loopback repro on macOS (both endpoints configured with the default 25 ms):

client / server MaxAckDelayMs after the transfer
main (5 runs) 26/26, 26/27, 26/27, 26/26, 26/27
this change (5 runs) 25/25 in all runs

Ran on macOS (arm64, quictls): msquiccoretest (all except the slow SpinFrame.* fuzz tests) passes apart from SettingsTest.GlobalExecutionConfigSetAndGet, which fails the same way on main on this machine, and the *Ack*:*Datagram*:*Send*:*Receive*:*Stream*:*Basic*:*Handshake* slice of msquictest passed 1757 of 1758 tests. The one failure was a single 2 s timeout in AppData/WithSend0RttArgs1.Send0Rtt/136; that case then passed 10 of 10 repeats, and the full Send0Rtt group passed in three further runs, so it looks like an intermittent timing failure rather than a regression.

Documentation

None.

The Requested Max Ack Delay in ACK_FREQUENCY frames was
QuicConnGetAckDelay(), the configured MaxAckDelayMs plus this endpoint's
timer resolution. The receiver adopts the requested value as its own
MaxAckDelayMs and adds its timer resolution again when it sends the frame
back, so the value grew by one timer tick per exchange (25, 41, 57, 73 ms
on Windows).

Request the configured MaxAckDelayMs instead, and never less than the
peer's min_ack_delay.

Add a test that checks MaxAckDelayMs is unchanged on both endpoints after
a transfer that makes the sender emit ACK_FREQUENCY frames.
@Kiptoo-Deus
Joel Kiptoo (Kiptoo-Deus) requested a review from a team as a code owner September 27, 2026 16:27

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant