fix(client): stop resize reconnects on the graphics pipeline

crates/ironrdp-client/src/rdp.rs

@@ -94,7 +94,7 @@ pub enum DisplayResizeFallbackReason {
     DisplayControlUnavailable,
     /// The server did not send the required Display Control capabilities PDU in time.
     CapabilitiesTimedOut,
-    /// The server did not reactivate the session after a monitor-layout request in time.
+    /// The server did not apply the requested desktop size through reactivation or graphics reset in time.
     ReactivationTimedOut,
 }
 
@@ -640,6 +640,9 @@ const DISPLAY_CONTROL_READY_TIMEOUT: Duration = Duration::from_secs(3);
 struct ResizeQueue {
     in_flight: Option<TimedResizeRequest>,
     pending: Option<TimedResizeRequest>,
+    /// The last request followed by a matching output reset or reactivation. Connection
+    /// parameters alone do not establish which layout the server accepted.
+    confirmed_layout: Option<ResizeRequest>,
 }
 
 impl ResizeQueue {
@@ -666,26 +669,82 @@ impl ResizeQueue {
         });
     }
 
-    fn completed(&mut self) {
-        self.in_flight = None;
+    fn completed(&mut self, desktop_size: (u16, u16)) -> bool {
+        if self
+            .in_flight
+            .as_ref()
+            .is_some_and(|in_flight| (in_flight.request.width, in_flight.request.height) == desktop_size)
+        {
+            self.confirmed_layout = self.in_flight.take().map(|in_flight| in_flight.request);
+            return true;
+        }
+
+        // An unsolicited or adjusted layout does not confirm an outstanding request.
+        self.confirmed_layout = None;
+        false
+    }
+
+    /// Whether a request repeats the last completed layout, with nothing in flight
+    /// that could change it. Servers can ignore such requests without sending a reset.
+    fn asks_for_current_layout(&self, request: &ResizeRequest, desktop_size: (u16, u16)) -> bool {
+        self.in_flight.is_none()
+            && (request.width, request.height) == desktop_size
+            && self.confirmed_layout == Some(*request)
+    }
+
+    fn pending_request(&mut self, desktop_size: (u16, u16)) -> Option<ResizeRequest> {
+        let request = self.pending.as_ref()?.request;
+        if self.asks_for_current_layout(&request, desktop_size) {
+            self.pending = None;
+            None
+        } else {
+            Some(request)
+        }
     }
 
-    fn timed_out_request(&self, now: tokio::time::Instant) -> Option<(ResizeRequest, DisplayResizeFallbackReason)> {
-        if let Some(in_flight) = self.in_flight.as_ref()
+    fn timed_out_request(
+        &mut self,
+        now: tokio::time::Instant,
+        desktop_size: (u16, u16),
+    ) -> Option<(ResizeRequest, DisplayResizeFallbackReason)> {
+        let expired = if let Some(in_flight) = self.in_flight.as_ref()
             && now >= in_flight.deadline
         {
-            return Some((
+            Some((
                 self.pending
                     .as_ref()
                     .map_or(in_flight.request, |pending| pending.request),
                 DisplayResizeFallbackReason::ReactivationTimedOut,
-            ));
-        }
+            ))
+        } else {
+            self.pending
+                .as_ref()
+                .filter(|pending| now >= pending.deadline)
+                .map(|pending| (pending.request, DisplayResizeFallbackReason::CapabilitiesTimedOut))
+        };
 
-        self.pending
-            .as_ref()
-            .filter(|pending| now >= pending.deadline)
-            .map(|pending| (pending.request, DisplayResizeFallbackReason::CapabilitiesTimedOut))
+        let (request, reason) = expired?;
+        if (request.width, request.height) == desktop_size
+            && self
+                .in_flight
+                .as_ref()
+                .is_none_or(|in_flight| (in_flight.request.width, in_flight.request.height) == desktop_size)
+        {
+            // Display Control has no explicit acknowledgement. A server can ignore
+            // scale/physical-size changes or a layout it already has (MS-RDPEDISP 1.3).
+            // Reconnecting to the same pixel size cannot recover those changes. Leave
+            // metadata unconfirmed so a later request can retry it.
+            debug!("Display layout metadata was not confirmed; keeping the current connection");
+            if self.in_flight.take().is_none() {
+                // Capabilities never arrived, so the metadata update cannot be sent.
+                self.pending = None;
+            }
+            // A newer deferred request still needs to be promoted and sent.
+            self.confirmed_layout = None;
+            None
+        } else {
+            Some((request, reason))
+        }
     }
 }
 
@@ -3119,6 +3178,7 @@ async fn active_session(
     let _ = clipboard_event_receiver;
 
     let disconnect_reason = 'outer: loop {
+        let framebuffer_size = (image.width(), image.height());
         let resize_deadline = resize_queue.deadline();
         let input_batch_deadline = input_batcher.deadline();
         let mut malformed_bitmap_redraw_queued = false;
@@ -3335,7 +3395,12 @@ async fn active_session(
                             scale_factor,
                             physical_size,
                         };
-                        if resize_queue.in_flight.is_some() || active_stage.display_control_ready() == Some(false) {
+                        if resize_queue.asks_for_current_layout(&request, framebuffer_size) {
+                            // This request also supersedes a deferred one.
+                            debug!(width, height, "Display already has the requested layout");
+                            resize_queue.pending = None;
+                            ActiveSessionIteration::outputs(Vec::new())
+                        } else if resize_queue.in_flight.is_some() || active_stage.display_control_ready() == Some(false) {
                             resize_queue.defer(request);
                             ActiveSessionIteration::outputs(Vec::new())
                         } else if let Some(dvc_batch) = active_stage.prepare_resize(
@@ -3356,6 +3421,10 @@ async fn active_session(
                                 }
                             }
                             ActiveSessionIteration::with_outputs(dvc_batch?, outputs)
+                        } else if (request.width, request.height) == framebuffer_size {
+                            debug!("Display Control is unavailable for a metadata-only update");
+                            resize_queue.pending = None;
+                            ActiveSessionIteration::outputs(Vec::new())
                         } else {
                             // TODO(#271): use the "auto-reconnect cookie": https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpbcgr/15b0d1c9-2891-4adb-a45e-deb4aeeeab7c
                             debug!("Reconnecting with new size");
@@ -3636,14 +3705,14 @@ async fn active_session(
                     None => core::future::pending().await,
                 }
                 } => {
-                let (request, reason) = resize_queue
-                    .timed_out_request(tokio::time::Instant::now())
-                    .expect("resize deadline must correspond to a queued request");
-                return Ok(RdpControlFlow::ReconnectWithNewSize {
-                    width: request.width,
-                    height: request.height,
-                    reason,
-                });
+                if let Some((request, reason)) = resize_queue.timed_out_request(tokio::time::Instant::now(), framebuffer_size) {
+                    return Ok(RdpControlFlow::ReconnectWithNewSize {
+                        width: request.width,
+                        height: request.height,
+                        reason,
+                    });
+                }
+                ActiveSessionIteration::outputs(Vec::new())
                 }
                 _ = async {
                     match rail_queue_release_deadline {
@@ -3705,6 +3774,19 @@ async fn active_session(
             }
         };
 
+        // ResetGraphics is an explicit completion signal even when a scale-only
+        // change leaves the pixel dimensions unchanged. Out-of-band resets must
+        // match the outstanding request before its layout is recorded as applied.
+        if let Some(output_size) = active_stage.take_graphics_output_reset()
+            && resize_queue.completed(output_size)
+        {
+            debug!(
+                width = output_size.0,
+                height = output_size.1,
+                "Graphics pipeline output reset completed the resize"
+            );
+        }
+
         if let Some(batch) = iteration.dvc_batch {
             let channel_id = batch.channel_id();
             let messages = batch.into_messages();
@@ -4060,7 +4142,7 @@ async fn active_session(
                             debug!(?desktop_size, "Deactivation-Reactivation Sequence completed");
                             image = DecodedImage::new(PixelFormat::RgbA32, desktop_size.width, desktop_size.height);
                             desktop_update_extent = None;
-                            resize_queue.completed();
+                            resize_queue.completed((desktop_size.width, desktop_size.height));
                             if !active_stage.reactivate(
                                 connection_activation.io_channel_id(),
                                 connection_activation.user_channel_id(),
@@ -4266,9 +4348,8 @@ async fn active_session(
         }
 
         if resize_queue.in_flight.is_none()
-            && let Some(pending) = resize_queue.pending.as_ref()
+            && let Some(request) = resize_queue.pending_request((image.width(), image.height()))
         {
-            let request = pending.request;
             match active_stage.display_control_ready() {
                 Some(true) => {
                     let batch = active_stage
@@ -4334,6 +4415,10 @@ async fn active_session(
                         }
                     }
                 }
+                None if (request.width, request.height) == (image.width(), image.height()) => {
+                    debug!("Display Control is unavailable for a metadata-only update");
+                    resize_queue.pending = None;
+                }
                 None => {
                     debug!("Reconnecting because Display Control is unavailable");
                     return Ok(RdpControlFlow::ReconnectWithNewSize {
@@ -4720,10 +4805,10 @@ mod tests {
         queue.defer(latest);
 
         assert_eq!(
-            queue.timed_out_request(deadline),
+            queue.timed_out_request(deadline, (800, 600)),
             Some((latest, DisplayResizeFallbackReason::ReactivationTimedOut))
         );
-        queue.completed();
+        assert!(queue.completed((1024, 768)));
         assert!(queue.in_flight.is_none());
         assert_eq!(queue.pending.as_ref().map(|pending| pending.request), Some(latest));
     }
@@ -4735,13 +4820,155 @@ mod tests {
         queue.defer(request);
         let deadline = queue.deadline().expect("pending resize must have a deadline");
 
-        assert_eq!(queue.timed_out_request(deadline - Duration::from_millis(1)), None);
         assert_eq!(
-            queue.timed_out_request(deadline),
+            queue.timed_out_request(deadline - Duration::from_millis(1), (800, 600)),
+            None
+        );
+        assert_eq!(
+            queue.timed_out_request(deadline, (800, 600)),
             Some((request, DisplayResizeFallbackReason::CapabilitiesTimedOut))
         );
     }
 
+    #[test]
+    fn resize_queue_recognizes_a_request_for_the_current_layout() {
+        let mut queue = ResizeQueue {
+            confirmed_layout: Some(resize_request(1024, 768)),
+            ..ResizeQueue::default()
+        };
+        let current = resize_request(1024, 768);
+        assert_eq!((current.scale_factor, current.physical_size), (100, None));
+
+        assert!(queue.asks_for_current_layout(&current, (1024, 768)));
+        assert!(!queue.asks_for_current_layout(&current, (1280, 720)));
+        let rescaled = ResizeRequest {
+            scale_factor: 150,
+            ..current
+        };
+        assert!(!queue.asks_for_current_layout(&rescaled, (1024, 768)));
+
+        // A request in flight can still change the layout, so nothing is a no-op meanwhile.
+        queue.mark_in_flight(rescaled);
+        assert!(!queue.asks_for_current_layout(&rescaled, (1024, 768)));
+        assert!(queue.completed((1024, 768)));
+        assert!(queue.asks_for_current_layout(&rescaled, (1024, 768)));
+        assert!(!queue.asks_for_current_layout(&current, (1024, 768)));
+    }
+
+    #[test]
+    fn resize_queue_requires_a_matching_server_size_before_recording_a_layout() {
+        let request = resize_request(1280, 720);
+        let mut queue = ResizeQueue::default();
+        assert!(!queue.asks_for_current_layout(&request, (1280, 720)));
+        queue.mark_in_flight(request);
+        assert_eq!(queue.confirmed_layout, None);
+        assert!(!queue.completed((1024, 768)));
+        assert_eq!(queue.in_flight.as_ref().map(|flight| flight.request), Some(request));
+        assert_eq!(queue.confirmed_layout, None);
+        assert!(queue.completed((1280, 720)));
+        assert!(queue.asks_for_current_layout(&request, (1280, 720)));
+    }
+
+    #[test]
+    fn resize_queue_drops_a_deferred_duplicate_after_completion() {
+        let request = resize_request(1280, 720);
+        let mut queue = ResizeQueue::default();
+        queue.mark_in_flight(request);
+        queue.defer(request);
+        assert!(queue.completed((1280, 720)));
+        assert_eq!(queue.pending_request((1280, 720)), None);
+        assert_eq!(queue.deadline(), None);
+
+        // A different deferred layout still needs to be sent.
+        let next = resize_request(1600, 900);
+        queue.defer(next);
+        assert_eq!(queue.pending_request((1280, 720)), Some(next));
+    }
+
+    #[test]
+    fn resize_queue_completes_scale_only_and_physical_size_changes() {
+        let current = resize_request(1024, 768);
+        for request in [
+            ResizeRequest {
+                scale_factor: 150,
+                ..current
+            },
+            ResizeRequest {
+                physical_size: Some((300, 200)),
+                ..current
+            },
+        ] {
+            let mut queue = ResizeQueue {
+                confirmed_layout: Some(current),
+                ..ResizeQueue::default()
+            };
+            assert!(!queue.asks_for_current_layout(&request, (1024, 768)));
+            queue.mark_in_flight(request);
+            // An explicit ResetGraphics can complete a request without changing pixels.
+            assert!(queue.completed((1024, 768)));
+            assert_eq!(queue.deadline(), None);
+            assert!(queue.asks_for_current_layout(&request, (1024, 768)));
+        }
+    }
+
+    #[test]
+    fn resize_queue_keeps_unacknowledged_metadata_retryable_without_reconnecting() {
+        let request = ResizeRequest {
+            scale_factor: 150,
+            ..resize_request(1024, 768)
+        };
+        let mut queue = ResizeQueue::default();
+        queue.mark_in_flight(request);
+        let deadline = queue.deadline().unwrap();
+        assert_eq!(queue.timed_out_request(deadline, (1024, 768)), None);
+        assert_eq!(queue.deadline(), None);
+        assert!(!queue.asks_for_current_layout(&request, (1024, 768)));
+
+        // A later pixel resize still uses the reconnect fallback if it times out.
+        queue.mark_in_flight(request);
+        let next = resize_request(1600, 900);
+        queue.defer(next);
+        let deadline = queue.in_flight.as_ref().unwrap().deadline;
+        assert_eq!(
+            queue.timed_out_request(deadline, (1024, 768)),
+            Some((next, DisplayResizeFallbackReason::ReactivationTimedOut))
+        );
+    }
+
+    #[test]
+    fn resize_queue_sends_newer_metadata_after_an_ignored_request() {
+        let first = ResizeRequest {
+            scale_factor: 150,
+            ..resize_request(1024, 768)
+        };
+        let latest = ResizeRequest {
+            scale_factor: 175,
+            ..first
+        };
+        let mut queue = ResizeQueue::default();
+        queue.mark_in_flight(first);
+        queue.defer(latest);
+        let deadline = queue.in_flight.as_ref().unwrap().deadline;
+        assert_eq!(queue.timed_out_request(deadline, (1024, 768)), None);
+        assert!(queue.in_flight.is_none());
+        assert_eq!(queue.pending_request((1024, 768)), Some(latest));
+    }
+
+    #[test]
+    fn resize_queue_preserves_fallback_for_a_return_to_the_original_size() {
+        let mut queue = ResizeQueue::default();
+        queue.mark_in_flight(resize_request(1280, 720));
+        let current = resize_request(1024, 768);
+        queue.defer(current);
+        let deadline = queue.in_flight.as_ref().unwrap().deadline;
+        // The earlier request can still change the desktop, so the pending request
+        // for its original size is not merely an unacknowledged metadata update.
+        assert_eq!(
+            queue.timed_out_request(deadline, (1024, 768)),
+            Some((current, DisplayResizeFallbackReason::ReactivationTimedOut))
+        );
+    }
+
     #[test]
     fn auto_reconnect_policy_requires_a_cookie_and_respects_its_limit() {
         let policy = AutoReconnectPolicy::new(2);