Wouldn't this violate the spec? Probe packets are congestion controlled.

For the record, the current behaviour follows RFC 9000, 9.4.

On confirming a peer's ownership of its new address, an endpoint MUST
immediately reset the congestion controller and round-trip time
estimator for the new path to initial values

Given that nginx uses a single CC context, the new path packets contribute to the existing CC state prior to path confirmation.

While I understand the rationale behind this patch, I cannot find a clause in any of the RFCs that allows this behavior. It seems that full cwnd should not be exceeded, which will result in path validation failure, followed by a new migration attempt.

This case originates from an issue we observed in production, which we can reliably reproduce with our test client. Enabling frame->ignore_congestion proved critical to resolving this stalling.

If PATH_CHALLENGE still needs to be congestion‑controlled, we may need a mechanism to buffer these packets when the congestion window is exhausted and send them immediately once space becomes available.

In Google Quiche, as soon as a non‑probing packet arrives on a new path, Quiche creates a new congestion‑control context, so it always has window capacity to send PATH_CHALLENGE. [As described below]: (https://github.com/google/quiche/blob/main/quiche/quic/core/quic_connection.cc#L5439) :

// If the new peer address share the same IP with the alternative path, the
// connection should switch to the congestion controller of the alternative
// path. Otherwise, the connection should use a brand new one.
// In order to re-use existing code in sent_packet_manager_, reset
// congestion controller to initial state first and then change to the one
// on alternative path.
// TODO(danzh) combine these two steps into one after deprecating gQUIC.

Nginx, by contrast, only resets its congestion control after receiving the corresponding PATH_RESPONSE.

Because congestion control is sender algorithm, and the PATH_CHALLENGE packets on the new path are very small and only 2–6 packets before the CC reset, this approach should have negligible impact on overall network performance.

@nandsky, RFC 9000, Section 9.4 says this:

On confirming a peer's ownership of its new address, an endpoint MUST immediately reset
the congestion controller...

This is exactly what nginx currently does. After receiving PATH_RESPONSE, nginx resets the congestion controller. Nothing is said about resetting it earlier. Later on, the same section says the following, which is exactly what nginx does (but apparently Quiche doesn't):

While multiple paths might be used during connection migration, a single congestion control
context and a single loss recovery context could be adequate. For instance, an endpoint might
delay switching to a new congestion control context until it is confirmed that an old path is no
longer needed (such as the case described in [Section 9.3.3]).

Regarding the Google Quiche, it looks like they indeed reset congestion control after receiving a non-probing packet from a new address. They save the old congestion control context in the alternative path and can get back to it later if the migration fails.

Also, they treat all in-flight packets as lost and retransmit them over the new path. This looks a bit strange to me since the new path is still unverified and can be a result of on-path/off-path attacks (RFC 9000, Section 9.3.2/9.3.3). However this alone can be enough to send PATH_CHALLENGE since after declaring all in-flight packets as lost, the full cwnd becomes available.

Having said that, I do recognize that the problem should be addressed and ignoring congestion control is probably the easiest way to do this. I will do some research of other QUIC software and get back here.

ngtcp2 seems to reset congestion control both on non-probing packet reception and later on path validation (required by RFC 9000):

https://github.com/ngtcp2/ngtcp2/blob/3355fcb0af4d5252999c1640132daab315af1a06/lib/ngtcp2_conn.c#L8434
https://github.com/ngtcp2/ngtcp2/blob/3355fcb0af4d5252999c1640132daab315af1a06/lib/ngtcp2_conn.c#L5752C7-L5752C34

I haven't found yet if they also declare all packets lost and retransmit them.

@arut @pluknet
I've discovered another issue: NGINX sends two types of PATH_CHALLENGE frames on a new path — the first with min == 0, and the second with min == 1200. This causes the client to respond with two PATH_RESPONSE frames.

However, when handling the PATH_RESPONSE, NGINX sets qc->rst_pnum to the latest received packet number.

Meanwhile, in ngx_quic_congestion_ack and ngx_quic_congestion_lost, if the acknowledged packet number (pnum) is less than qc->rst_pnum, NGINX does not subtract the corresponding inflight bytes. As a result, data sent between the two PATH_CHALLENGE frames on the new path remains counted toward the congestion window, even after it has been acknowledged.

Below is a sequence diagram based on SystemTap that illustrates the issue:

quic send pn:135 rst_pn:0
handle path_response path seq:0 reset congestion window and rst pnum:0 path state:1
quic send pn:137 rst_pn:0
quic send pn:138 rst_pn:0
quic send pn:139 rst_pn:0
t:1753467101871 pn:137 fr:6-130 ack:3(3158 bytes) lost:0(0 bytes)
quic send pn:140 rst_pn:0
quic send pn:142 rst_pn:0
quic send pn:143 rst_pn:0
quic send pn:144 rst_pn:0
handle path_response path seq:1 reset congestion window and rst pnum:145 path state:1
quic send pn:145 rst_pn:145
quic send pn:146 rst_pn:145
quic send pn:148 rst_pn:145
quic send pn:149 rst_pn:145
quic send pn:150 rst_pn:145
quic send pn:151 rst_pn:145
quic send pn:152 rst_pn:145
quic send pn:153 rst_pn:145
quic send pn:154 rst_pn:145
quic send pn:155 rst_pn:145
quic send pn:156 rst_pn:145
quic send pn:157 rst_pn:145
t:1753467101871 pn:140 fr:6-132 ack:0(0 bytes) lost:0(0 bytes)
t:1753467101872 pn:142 fr:117-135 ack:0(0 bytes) lost:0(0 bytes)
quic send pn:159 rst_pn:145
quic send pn:160 rst_pn:145
quic send pn:161 rst_pn:145
quic send pn:162 rst_pn:145
quic send pn:163 rst_pn:145
quic send pn:164 rst_pn:145
quic send pn:165 rst_pn:145
quic send pn:166 rst_pn:145
quic send pn:167 rst_pn:145
quic send pn:168 rst_pn:145
t:1753467101892 pn:143 fr:117-141 ack:0(0 bytes) lost:0(0 bytes)
handle path_response path seq:1 reset congestion window and rst pnum:169 path state:1
t:1753467101892 pn:145 fr:117-145 ack:0(0 bytes) lost:0(0 bytes)
quic send pn:170 rst_pn:169
t:1753467101894 pn:147 fr:117-158 ack:0(0 bytes) lost:0(0 bytes)
t:1753467101895 pn:148 fr:117-160 ack:0(0 bytes) lost:0(0 bytes)
quic send pn:171 rst_pn:169
quic send pn:172 rst_pn:169
quic send pn:173 rst_pn:169
t:1753467101900 pn:149 fr:117-162 ack:0(0 bytes) lost:0(0 bytes)
t:1753467101904 pn:150 fr:117-164 ack:0(0 bytes) lost:0(0 bytes)
t:1753467101908 pn:151 fr:117-166 ack:0(0 bytes) lost:0(0 bytes)
t:1753467101911 pn:152 fr:117-168 ack:0(0 bytes) lost:0(0 bytes)
t:1753467101915 pn:153 fr:117-171 ack:2(2756 bytes) lost:0(0 bytes)
quic send pn:174 rst_pn:169
quic send pn:175 rst_pn:169

I think this is a correct change in general, due to a separate timer maintained for PATH_CHALLENGE frames.

Agreed, the patch itself looks ok. The subject line exceeds 67 characters though.