From df2f441014a46b62851eee5d2564140efcd86107 Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Sat, 12 Sep 2026 15:21:25 -0500 Subject: [PATCH 1/3] fix(server): log bandwidth measure's raw inputs, not just the figure bandwidth_kbps alone reads as noise on a damage-driven video source: a single 1.25s measurement window catches either near-idle traffic (a handful of cursor/autodetect PDUs, ~1.8KB) or a real EGFX frame landing in it (tens of KB), so the same unthrottled link reports anywhere from ~11kbps to ~600kbps depending on what the encoder happened to be doing in that specific window. Logging time_delta_ms/byte_count alongside the computed figure makes that bimodality visible instead of looking like a calculation bug -- the formula (byte_count*8/time_delta_ms) was already correct; the volatility is inherent to counting real traffic over a short window against a bursty source, not a defect. --- crates/ironrdp-server/src/server.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/crates/ironrdp-server/src/server.rs b/crates/ironrdp-server/src/server.rs index f33f99bae3..aaa45af2e0 100644 --- a/crates/ironrdp-server/src/server.rs +++ b/crates/ironrdp-server/src/server.rs @@ -3872,8 +3872,24 @@ impl RdpServer { } AutoDetectOutcome::Bandwidth(Some(bandwidth_kbps)) => { self.autodetect_bandwidth.store(bandwidth_kbps, Ordering::Relaxed); + // Logging the raw inputs, not just the computed figure: a + // damage-driven video source makes any single measurement + // window's byte count wildly bimodal (near-idle vs. a real + // frame landing in it), so bandwidth_kbps alone reads as + // noise without time_delta_ms/byte_count alongside it to + // show why. + let rdp::autodetect::AutoDetectResponse::BandwidthMeasureResults { + time_delta_ms, + byte_count, + .. + } = &pdu.response + else { + unreachable!("computed_bandwidth_kbps() only returns Some for this variant") + }; debug!( bandwidth_kbps, + time_delta_ms, + byte_count, seq = pdu.response.sequence_number(), "Bandwidth measured" ); From ee87cf195efd46ec5c4ca78949696853598b8711 Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Sat, 12 Sep 2026 18:22:30 -0500 Subject: [PATCH 2/3] feat(server): add bandwidth-measure generation counter The bandwidth figure alone repeats too often to tell a fresh measurement window apart from a stale one (a quiet link reads the same low figure for several consecutive windows). Expose a counter that increments on every completed Bandwidth Measure transaction, successful or not, so a consumer can gate its own filtering logic on "a new window just closed" instead of diffing the value itself. --- crates/ironrdp-server/src/builder.rs | 16 ++++++++++++++++ crates/ironrdp-server/src/server.rs | 27 +++++++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/crates/ironrdp-server/src/builder.rs b/crates/ironrdp-server/src/builder.rs index e54e53a1fc..c61fc3c9d8 100644 --- a/crates/ironrdp-server/src/builder.rs +++ b/crates/ironrdp-server/src/builder.rs @@ -60,6 +60,7 @@ pub struct BuilderDone { autodetect_rtt: Option>, autodetect_baseline_rtt: Option>, autodetect_bandwidth: Option>, + autodetect_bandwidth_generation: Option>, honor_client_desktop_size: Option, auto_reconnect_cookie: Option, connection_policy: ConnectionPolicy, @@ -173,6 +174,7 @@ impl RdpServerBuilder { autodetect_rtt: None, autodetect_baseline_rtt: None, autodetect_bandwidth: None, + autodetect_bandwidth_generation: None, honor_client_desktop_size: None, connection_policy: ConnectionPolicy::default(), auto_reconnect_cookie: None, @@ -207,6 +209,7 @@ impl RdpServerBuilder { autodetect_rtt: None, autodetect_baseline_rtt: None, autodetect_bandwidth: None, + autodetect_bandwidth_generation: None, honor_client_desktop_size: None, connection_policy: ConnectionPolicy::default(), auto_reconnect_cookie: None, @@ -418,6 +421,18 @@ impl RdpServerBuilder { self } + /// Inject a shared handle that increments every time a Bandwidth Measure + /// transaction completes, whether or not it produced a usable figure. + /// Pairs with [`Self::with_autodetect_bandwidth_handle`]: the bandwidth + /// figure alone repeats too often to tell a fresh measurement window + /// apart from a stale one. When not called, the server allocates its own + /// (still readable via + /// [`RdpServer::autodetect_bandwidth_generation_handle`]). + pub fn with_autodetect_bandwidth_generation_handle(mut self, handle: Arc) -> Self { + self.state.autodetect_bandwidth_generation = Some(handle); + self + } + /// Provision the Server Auto-Reconnect Cookie (MS-RDPBCGR 2.2.4.2 /// `ARC_SC_PRIVATE_PACKET`) handed to the client during logon. /// @@ -493,6 +508,7 @@ impl RdpServerBuilder { self.state.autodetect_rtt, self.state.autodetect_baseline_rtt, self.state.autodetect_bandwidth, + self.state.autodetect_bandwidth_generation, ); server.set_credential_validator(self.state.credential_validator); server.set_auto_reconnect_cookie(self.state.auto_reconnect_cookie); diff --git a/crates/ironrdp-server/src/server.rs b/crates/ironrdp-server/src/server.rs index aaa45af2e0..1d5138ce8b 100644 --- a/crates/ironrdp-server/src/server.rs +++ b/crates/ironrdp-server/src/server.rs @@ -743,6 +743,16 @@ pub struct RdpServer { /// alone does not fix. autodetect_bandwidth: Arc, + /// Increments every time a Bandwidth Measure transaction completes + /// (whether or not it produced a usable figure — see + /// [`Self::autodetect_bandwidth`]'s doc comment on the None case). + /// [`Self::autodetect_bandwidth`] alone cannot tell an embedder "a new + /// window just closed" apart from "the value happens to repeat" — this + /// value repeats often (a quiet link reads the same low figure for + /// several consecutive windows), so diffing it is not a valid freshness + /// signal. Exposed via [`Self::autodetect_bandwidth_generation_handle`]. + autodetect_bandwidth_generation: Arc, + /// Optional Server Auto-Reconnect Cookie (MS-RDPBCGR 2.2.4.2 /// `ARC_SC_PRIVATE_PACKET`). When `Some`, the server validates a returning /// `ARC_CS_PRIVATE_PACKET`, replaces its random after every connection, and @@ -1375,6 +1385,7 @@ impl RdpServer { autodetect_rtt: Option>, autodetect_baseline_rtt: Option>, autodetect_bandwidth: Option>, + autodetect_bandwidth_generation: Option>, ) -> Self { let (ev_sender, ev_receiver) = ServerEvent::create_channel(); if let Some(cliprdr) = cliprdr_factory.as_mut() { @@ -1441,6 +1452,8 @@ impl RdpServer { handle.store(u32::MAX, Ordering::Relaxed); handle }, + autodetect_bandwidth_generation: autodetect_bandwidth_generation + .unwrap_or_else(|| Arc::new(AtomicU32::new(0))), auto_reconnect_cookie: None, previous_auto_reconnect_cookie: None, auto_reconnect_sent: false, @@ -1783,6 +1796,18 @@ impl RdpServer { Arc::clone(&self.autodetect_bandwidth) } + /// Returns a handle that increments every time a Bandwidth Measure + /// transaction completes, whether or not it produced a usable figure. + /// Pairs with [`Self::autodetect_bandwidth_handle`]: read this first to + /// detect a fresh measurement window (the bandwidth figure itself + /// repeats too often to be its own freshness signal), then read the + /// bandwidth handle for the value. Inject a shared instance at + /// construction with + /// [`RdpServerBuilder::with_autodetect_bandwidth_generation_handle`](crate::RdpServerBuilder::with_autodetect_bandwidth_generation_handle). + pub fn autodetect_bandwidth_generation_handle(&self) -> Arc { + Arc::clone(&self.autodetect_bandwidth_generation) + } + /// Returns the shared ECHO server handle for runtime probe requests and RTT measurements. pub fn echo_handle(&self) -> &EchoServerHandle { &self.echo_handle @@ -3872,6 +3897,7 @@ impl RdpServer { } AutoDetectOutcome::Bandwidth(Some(bandwidth_kbps)) => { self.autodetect_bandwidth.store(bandwidth_kbps, Ordering::Relaxed); + self.autodetect_bandwidth_generation.fetch_add(1, Ordering::Relaxed); // Logging the raw inputs, not just the computed figure: a // damage-driven video source makes any single measurement // window's byte count wildly bimodal (near-idle vs. a real @@ -3899,6 +3925,7 @@ impl RdpServer { // reporting a stale one (see `handle_response`'s doc comment); // mirror that here so the exposed handle does not disagree. self.autodetect_bandwidth.store(u32::MAX, Ordering::Relaxed); + self.autodetect_bandwidth_generation.fetch_add(1, Ordering::Relaxed); trace!( seq = pdu.response.sequence_number(), "Bandwidth measurement completed without a usable figure" From fcab3b7378d1ef027a2fcd92c24749cc912641f6 Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Tue, 22 Sep 2026 19:23:34 -0500 Subject: [PATCH 3/3] review: publish the bandwidth generation with Release and drop the log panic path The generation counter is now incremented with Release after the bandwidth store, and the accessor documents an Acquire load and an at-least-as-new guarantee rather than an exact pair. The bandwidth log records the whole response instead of destructuring it behind an unreachable!(). Adds default and injection tests for the generation handle. --- crates/ironrdp-server/src/server.rs | 50 ++++++++---------- .../tests/server/autodetect.rs | 52 +++++++++++++++++++ 2 files changed, 73 insertions(+), 29 deletions(-) diff --git a/crates/ironrdp-server/src/server.rs b/crates/ironrdp-server/src/server.rs index 1d5138ce8b..f85c94d32b 100644 --- a/crates/ironrdp-server/src/server.rs +++ b/crates/ironrdp-server/src/server.rs @@ -743,14 +743,16 @@ pub struct RdpServer { /// alone does not fix. autodetect_bandwidth: Arc, - /// Increments every time a Bandwidth Measure transaction completes - /// (whether or not it produced a usable figure — see + /// Increments every time a Bandwidth Measure transaction completes, + /// whether or not it produced a usable figure (see /// [`Self::autodetect_bandwidth`]'s doc comment on the None case). /// [`Self::autodetect_bandwidth`] alone cannot tell an embedder "a new - /// window just closed" apart from "the value happens to repeat" — this + /// window just closed" apart from "the value happens to repeat": that /// value repeats often (a quiet link reads the same low figure for /// several consecutive windows), so diffing it is not a valid freshness - /// signal. Exposed via [`Self::autodetect_bandwidth_generation_handle`]. + /// signal. Incremented with `Release` after the bandwidth value is + /// stored, so an `Acquire` load of it makes that value visible. + /// Exposed via [`Self::autodetect_bandwidth_generation_handle`]. autodetect_bandwidth_generation: Arc, /// Optional Server Auto-Reconnect Cookie (MS-RDPBCGR 2.2.4.2 @@ -1798,11 +1800,15 @@ impl RdpServer { /// Returns a handle that increments every time a Bandwidth Measure /// transaction completes, whether or not it produced a usable figure. - /// Pairs with [`Self::autodetect_bandwidth_handle`]: read this first to - /// detect a fresh measurement window (the bandwidth figure itself - /// repeats too often to be its own freshness signal), then read the - /// bandwidth handle for the value. Inject a shared instance at - /// construction with + /// Pairs with [`Self::autodetect_bandwidth_handle`]: load this with + /// `Ordering::Acquire` to detect a fresh measurement window (the + /// bandwidth figure itself repeats too often to be its own freshness + /// signal), then read the bandwidth handle for the value. The server + /// increments this with `Ordering::Release` after storing the value, so + /// the bandwidth read is at least as new as the generation observed. It + /// is not an exact pair: If the next window closes between the two reads, + /// the value can already belong to that later window. Inject a shared + /// instance at construction with /// [`RdpServerBuilder::with_autodetect_bandwidth_generation_handle`](crate::RdpServerBuilder::with_autodetect_bandwidth_generation_handle). pub fn autodetect_bandwidth_generation_handle(&self) -> Arc { Arc::clone(&self.autodetect_bandwidth_generation) @@ -3897,35 +3903,21 @@ impl RdpServer { } AutoDetectOutcome::Bandwidth(Some(bandwidth_kbps)) => { self.autodetect_bandwidth.store(bandwidth_kbps, Ordering::Relaxed); - self.autodetect_bandwidth_generation.fetch_add(1, Ordering::Relaxed); - // Logging the raw inputs, not just the computed figure: a + self.autodetect_bandwidth_generation.fetch_add(1, Ordering::Release); + // Logging the whole response, not just the computed figure: a // damage-driven video source makes any single measurement // window's byte count wildly bimodal (near-idle vs. a real // frame landing in it), so bandwidth_kbps alone reads as - // noise without time_delta_ms/byte_count alongside it to - // show why. - let rdp::autodetect::AutoDetectResponse::BandwidthMeasureResults { - time_delta_ms, - byte_count, - .. - } = &pdu.response - else { - unreachable!("computed_bandwidth_kbps() only returns Some for this variant") - }; - debug!( - bandwidth_kbps, - time_delta_ms, - byte_count, - seq = pdu.response.sequence_number(), - "Bandwidth measured" - ); + // noise without the response's time_delta_ms/byte_count + // alongside it to show why. + debug!(bandwidth_kbps, response = ?pdu.response, "Bandwidth measured"); } AutoDetectOutcome::Bandwidth(None) => { // The manager just cleared its own figure rather than keep // reporting a stale one (see `handle_response`'s doc comment); // mirror that here so the exposed handle does not disagree. self.autodetect_bandwidth.store(u32::MAX, Ordering::Relaxed); - self.autodetect_bandwidth_generation.fetch_add(1, Ordering::Relaxed); + self.autodetect_bandwidth_generation.fetch_add(1, Ordering::Release); trace!( seq = pdu.response.sequence_number(), "Bandwidth measurement completed without a usable figure" diff --git a/crates/ironrdp-testsuite-core/tests/server/autodetect.rs b/crates/ironrdp-testsuite-core/tests/server/autodetect.rs index 7a94d2e22e..8b1e6a6391 100644 --- a/crates/ironrdp-testsuite-core/tests/server/autodetect.rs +++ b/crates/ironrdp-testsuite-core/tests/server/autodetect.rs @@ -543,6 +543,58 @@ fn with_autodetect_bandwidth_handle_round_trips_the_same_arc() { assert_eq!(server.autodetect_bandwidth_handle().load(Ordering::Relaxed), 42); } +#[test] +fn autodetect_bandwidth_generation_handle_defaults_to_zero() { + use core::net::{Ipv4Addr, SocketAddr}; + use core::sync::atomic::Ordering; + + use ironrdp_server::RdpServer; + + let server = RdpServer::builder() + .with_addr(SocketAddr::from((Ipv4Addr::LOCALHOST, 0))) + .with_no_security() + .with_no_input() + .with_no_display() + .build(); + + assert_eq!( + server.autodetect_bandwidth_generation_handle().load(Ordering::Acquire), + 0 + ); +} + +#[test] +fn with_autodetect_bandwidth_generation_handle_round_trips_the_same_arc() { + use core::net::{Ipv4Addr, SocketAddr}; + use core::sync::atomic::{AtomicU32, Ordering}; + use std::sync::Arc; + + use ironrdp_server::RdpServer; + + let handle = Arc::new(AtomicU32::new(7)); + let server = RdpServer::builder() + .with_addr(SocketAddr::from((Ipv4Addr::LOCALHOST, 0))) + .with_no_security() + .with_no_input() + .with_no_display() + .with_autodetect_bandwidth_generation_handle(Arc::clone(&handle)) + .build(); + + assert!(Arc::ptr_eq(&handle, &server.autodetect_bandwidth_generation_handle())); + // Unlike the bandwidth value, an injected generation counter is not reset at + // construction: an embedder sharing one counter across servers keeps counting. + assert_eq!( + server.autodetect_bandwidth_generation_handle().load(Ordering::Acquire), + 7 + ); + // The Arc is shared: advancing the original is visible through the server's handle. + handle.fetch_add(1, Ordering::Release); + assert_eq!( + server.autodetect_bandwidth_generation_handle().load(Ordering::Acquire), + 8 + ); +} + #[test] fn stale_probe_expiry() { let mut mgr = AutoDetectManager::new();