PATH_CHALLENGE is never sent

[nemethf]

My client host has two network interfaces. I bind the client to 0.0.0.0:0 and bring down the primary interface with ip link set dev eth0 down during a download. This does not result in a connection migration.

Firstly, because the client has nothing to send, so the server won't be notified about the new path. So I change the client-config to make it send pings periodically:

    let mut transport_config = TransportConfig::default();
    transport_config
        .keep_alive_interval(Some(Duration::from_secs(1)));
    client_config.transport_config(Arc::new(transport_config));

Now when the server notices the new path, it creates two path challenges in Connection::migrate(), one for the prev_path, one for the new_path. However, it never sends the PATH_CHALLENGE for new_path. I'm guessing this is because poll_transmit() always has something better to send. If I modify poll_transmit to prioritize both PATH_CHALLENGEs, then the connection migration works as expected.

I copy my patch below. But most probably this is not the right fix, because probably it is not a good idea to send a PATH_CHALLENGE frame alone in a packet and before other more important frames.

diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs
index 03a6050..25b2631 100644
--- a/quinn-proto/src/connection/mod.rs
+++ b/quinn-proto/src/connection/mod.rs
@@ -519,6 +519,54 @@ impl Connection {
                 });
             }
         }
+        // Send PATH_CHALLENGE for a current path if necessary
+        if self.path.challenge_pending {
+            self.path.challenge_pending = false;
+            let token = self.path
+                .challenge
+                .expect("path challenge pending without token");
+            let destination = self.path.remote;
+            debug_assert_eq!(
+                self.highest_space,
+                SpaceId::Data,
+                "PATH_CHALLENGE queued without 1-RTT keys"
+            );
+            buf.reserve(self.path.current_mtu() as usize);
+
+            let buf_capacity = buf.capacity();
+
+            let mut builder = PacketBuilder::new(
+                now,
+                SpaceId::Data,
+                buf,
+                buf_capacity,
+                0,
+                false,
+                self,
+                self.version,
+            )?;
+            trace!("validating path with PATH_CHALLENGE {:08x}", token);
+            buf.write(frame::Type::PATH_CHALLENGE);
+            buf.write(token);
+            self.stats.frame_tx.path_challenge += 1;
+
+            // An endpoint MUST expand datagrams that contain a
+            // PATH_CHALLENGE frame to at least the smallest allowed
+            // maximum datagram size of 1200 bytes, unless the
+            // anti-amplification limit for the path does not permit
+            // sending a datagram of this size
+            builder.pad_to(MIN_INITIAL_SIZE);
+
+            builder.finish(self, buf);
+            self.stats.udp_tx.on_sent(1, buf.len());
+            return Some(Transmit {
+                destination,
+                size: buf.len(),
+                ecn: None,
+                segment_size: None,
+                src_ip: self.local_ip,
+            });
+        }

         // If we need to send a probe, make sure we have something to send.
         for space in SpaceId::iter() {

[djc]

However, it never sends the PATH_CHALLENGE for new_path. I'm guessing this is because poll_transmit() always has something better to send. If I modify poll_transmit to prioritize both PATH_CHALLENGEs, then the connection migration works as expected.

Looking at populate_packet() it seems to actually prioritize PATH_CHALLENGE a decent amount? In particular, it should definitely be sent before DATAGRAM or STREAM frames, so it's surprising that this doesn't happen. Are you sure other packets are being sent on the new path? Can you dig into how/why we're not hitting the populate_packet() if let Some(token) = self.path.challenge { path?

[nemethf]

Can you dig into how/why we're not hitting the populate_packet() if let Some(token) = self.path.challenge { path?

This might not be the only reason, but this if condition evaluates to true:

                    if self.in_flight.bytes + bytes_to_send >= self.path.congestion.window() {

It definitely seems strange. Cwnd is per path, which is okay. But the server should be able to send a path_challenge even if there are lots of unacknowledged packets on prev_path. And since the prev_path is unreachable, the situation won't change. (Well, maybe it will change when a timeout occurs for the in-flight packets, but by that time the unsent path_challenge will expire as well. Or so it seems.)

[djc]

But that happens after the migration, right, at which point self.path should contain the new path?

[nemethf]

That's right: self.path is the new path, but in_flight contains info from the previous path as well. In my case: in_flight:20328 bytes_to_send:1200 cwnd:12000 space_idx:2.

[djc]

Okay, so because we haven't seen ACKs from the previous path we don't want to send more data, but we need to challenge/confirm the new path in order to be able to continue receiving ACKs?

[nemethf]

Yes, that's my conclusion. It sounds plausible to me.

(Also, those ACKs might never arrive since the client might never receive the corresponding packets.)

[Ralith]

it never sends the PATH_CHALLENGE for new_path

How long did you wait? The path validation timeout is equal to 3 probe timeouts, so there should be plenty of time for the server to send a tail loss probe, receive an ACK, and free up congestion window space by judging packets as delivered or lost in response.

Even if that does occur, though, this behavior seems needlessly pessimal. Perhaps we should track "bytes in flight" per path rather than globally, ensuring that we can always send immediately on a new path. Will discuss in the implementers' slack to make sure I'm not missing something.

[nemethf]

it never sends the PATH_CHALLENGE for new_path

How long did you wait? The path validation timeout is equal to 3 probe timeouts, so there should be plenty of time for the server to send a tail loss probe, receive an ACK, and free up congestion window space by judging packets as delivered or lost in response.

I don't know, but I did see the creation of path challenges three times in migrate(). I think the problem is that even if the server can send something on the old path, the client won't be able to receive it, and therefore won't send an ACK back.

Even if that does occur, though, this behavior seems needlessly pessimal.

Exactly. Although I haven't measured it, my patch in the original issue description was able to bring down the outage to a reasonably small duration. (When a manual address change initiates the migration and both paths are operational, the outage is even smaller.)

Perhaps we should track "bytes in flight" per path rather than globally, ensuring that we can always send immediately on a new path.

If "bytes in flight" are used in flow-control as well, then I think naively maintaining per path values has a danger of overloading the receiver.

[Ralith]

I think the problem is that even if the server can send something on the old path, the client won't be able to receive it, and therefore won't send an ACK back.

The server should send a tail loss probe (and receive the resulting ACK) on the new path. Because the PTO is shorter than the path validation time-out, so the new path should still be active when the PTO expires.

If "bytes in flight" are used in flow-control as well

They are not. Congestion control and flow control are independent.

[nemethf]

The server should send a tail loss probe (and receive the resulting ACK) on the new path. Because the PTO is shorter than the path validation time-out, so the new path should still be active when the PTO expires.

I've attached a .pcap file. Maybe it helps to reveal what's going on. It was captured on the server size. Packet No. 787 is the client's first packet arriving to the server after the cut of primary path.

test.h2.pcapng.zip