Skip to content

feat(server): expose measured bandwidth to the embedder - #1734

Open
Greg Lamberson (glamberson) wants to merge 2 commits into
Devolutions:masterfrom
lamco-admin:feat/server-bandwidth-handle
Open

feat(server): expose measured bandwidth to the embedder#1734
Greg Lamberson (glamberson) wants to merge 2 commits into
Devolutions:masterfrom
lamco-admin:feat/server-bandwidth-handle

Conversation

@glamberson

Copy link
Copy Markdown
Contributor

Summary

  • After feat(server): measure and report network characteristics #1470/feat(server): measure bandwidth and report it in NetworkCharacteristicsResult #1471, the server can tell the client its measured
    bandwidth over the wire (Network Characteristics Result), but nothing
    exposes that figure outside the crate: snapshot() returns
    RttSnapshot (min/max/avg/sample_count only) and
    autodetect_rtt_handle() carries RTT alone. An embedder's own
    health or flow-control layer that wants the same bandwidth figure
    the client already received has no way to read it.
  • Mirrors the existing autodetect_rtt_handle shape exactly: a new
    autodetect_bandwidth Arc<AtomicU32> field (u32::MAX sentinel
    until the first measurement, matching autodetect_rtt), an
    autodetect_bandwidth_handle() accessor, and a
    with_autodetect_bandwidth_handle() builder method for injecting a
    shared instance. Adds AutoDetectManager::bandwidth_kbps() as the
    underlying getter; the field already existed internally with no
    public accessor.
  • The store site needed a before/after comparison rather than a plain
    else branch on handle_response's result: that function reports a
    matched RTT sample through its return value but only updates
    bandwidth internally, so a naive else would also fire, and
    mislabel the store as a fresh bandwidth measurement, on any
    unmatched or stray RTT response once a bandwidth figure had been
    recorded at least once. Comparing bandwidth_kbps() before and
    after the call only stores and logs when a measurement genuinely
    completed.
  • Adds three tests to the existing autodetect test file, mirroring the
    RTT-handle tests already there: bandwidth_kbps() reflecting a
    completed measurement, the handle's sentinel default, and the
    injected-handle round trip.

Validation

cargo xtask check fmt/lints/tests/typos/locks all pass, including the
3 new tests (22/22 passing in server::autodetect).

Notes

No public API break: RdpServer::new is crate-private, and the public
surface (RdpServerBuilder) only gains an additive optional field and a
new method.

After Devolutions#1470/Devolutions#1471, the server can tell the client its measured
bandwidth over the wire (Network Characteristics Result), but nothing
exposes that figure outside the crate. snapshot() returns RttSnapshot
(min_ms/max_ms/avg_ms/sample_count only), and autodetect_rtt_handle()
carries RTT alone. An embedder's own health/flow-control layer that
wants the same bandwidth figure the client already received has no way
to read it.

Mirror the existing autodetect_rtt_handle shape exactly: a new
autodetect_bandwidth Arc<AtomicU32> field (u32::MAX sentinel until the
first measurement, matching autodetect_rtt), an
autodetect_bandwidth_handle() accessor, and a
with_autodetect_bandwidth_handle() builder method for injecting a
shared instance. Add AutoDetectManager::bandwidth_kbps() as the
underlying getter; the field already existed internally but had no
public accessor at all.

The store site needs a before/after comparison rather than a plain
else branch on handle_response's result: that function reports a
matched RTT sample through its return value but only updates bandwidth
internally (per its own doc comment), so a naive else would also fire,
and mislabel the store as a fresh bandwidth measurement, on any
unmatched or stray RTT response once a bandwidth figure had been
recorded at least once. Comparing bandwidth_kbps() before and after the
call only stores and logs when a measurement genuinely completed.

Adds three tests to the existing autodetect test file, mirroring the
RTT-handle tests already there: bandwidth_kbps() reflecting a completed
measurement, the handle's sentinel default, and the injected-handle
round-trip.
@github-actions github-actions Bot added kind/protocol Affects RDP or related protocol behavior maintainer-required Maintainer review or intervention is required risk/medium Behavioral change that does not substantially alter a core public API size/S Size: up to 199 counted lines and 5 files; exceeds XS in either measure ai-reviewed/1 One automated review completed and removed maintainer-required Maintainer review or intervention is required labels Aug 21, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additive, well-scoped feature mirroring the existing autodetect_rtt_handle pattern (new builder field, accessor, handle-injection method, plus a getter on AutoDetectManager). One real correctness defect in the server.rs dispatch logic: using a before/after equality check on bandwidth_kbps() to detect a completed measurement misclassifies two legitimate cases as an unmatched auto-detect response — consecutive measurements that compute the same kbps value, and a matched-but-unusable measurement (time_delta_ms == 0) that internally clears bandwidth_kbps to None while leaving the exposed atomic stale indefinitely.

Protocol analysis: accepted — Independently traced handle_response and computed_bandwidth_kbps: time_delta_ms == 0 returns None and the manager clears bandwidth_kbps internally. Confirmed the handoff's 'ambiguous' discrepancy: a matched-but-unusable result is logged as unmatched and the atomic is never cleared, going stale. I also found a more probable trigger for the same root cause: two consecutive successful measurements with an identical kbps value likewise take the 'Unmatched' branch, since before/after value-equality is a weaker test than 'did handle_response consume a matched bandwidth response'. Reported as one finding.

  1. non_blocking / low — crates/ironrdp-testsuite-core/tests/server/autodetect.rs
    New tests cover only the sentinel default, handle round-trip, and one successful measurement. Neither scenario that breaks the before/after detection logic (a time_delta_ms == 0 matched-but-unusable result, or two consecutive measurements with an identical kbps figure) is exercised, though both are reachable from client-controlled wire input.

Comment thread crates/ironrdp-server/src/server.rs
handle_response detected a completed bandwidth measurement by comparing
bandwidth_kbps() before and after the call rather than from its own
return value, since only RTT was signaled through it. That comparison
misses two real cases: two consecutive measurements landing on the
same kbps figure leave the value unchanged, so a real match reads as
unmatched, and a measurement with time_delta_ms == 0 clears the
internal figure to None, which the comparison also can't tell apart
from no match at all. The second case left the exposed atomic serving
a stale figure the manager had already discarded, defeating the
withhold-rather-than-report-stale policy handle_response's own doc
comment already describes.

Change handle_response's return type from Option<u32> to an explicit
AutoDetectOutcome (Rtt, Bandwidth, Unmatched), so the dispatch match in
server.rs is exhaustive and direct instead of derived from a value
comparison. On Bandwidth(None), reset the exposed atomic to the
sentinel too, so it agrees with the manager's own cleared state instead
of holding onto the last good figure indefinitely.

Adds a test for two consecutive identical-value measurements, and
tightens the existing time_delta_ms: 0 test to assert Bandwidth(None)
specifically rather than is_none() on the old return type, which could
not distinguish matched-but-unusable from unmatched.

Public API break on AutoDetectManager::handle_response's return type.
@github-actions github-actions Bot added breaking-change Includes a breaking change, and requires special scrutiny at the boundaries maintainer-required Maintainer review or intervention is required risk/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny size/M Size: up to 449 counted lines and 10 files; exceeds S in either measure and removed risk/medium Behavioral change that does not substantially alter a core public API size/S Size: up to 199 counted lines and 5 files; exceeds XS in either measure labels Aug 21, 2026
Marc-André Moreau (mamoreau-devolutions) pushed a commit that referenced this pull request Aug 21, 2026
…1737)

## Summary

- RttSnapshot.min_ms is a sliding-window low that can rise as low
samples
  age out of the window, and is explicitly documented as not baseRTT per
  MS-RDPBCGR 2.2.14.1.5, which defines baseRTT as the session-lifetime
  lowest. AutoDetectManager already tracks that true low internally as
  min_rtt_ms for the wire NetworkCharacteristicsResult, but nothing
  exposed it as its own value: an embedder reading snapshot() or
  autodetect_rtt_handle() alone cannot derive averageRTT - baseRTT as a
  queueing-delay signal, since that relationship only holds when the
  floor cannot rise.
- Adds AutoDetectManager::baseline_rtt_ms() as the public getter for the
  existing field, and mirrors the autodetect_rtt_handle plumbing on
  RdpServer: a new autodetect_baseline_rtt: Arc<AtomicU32> field,
  autodetect_baseline_rtt_handle() accessor, and
  with_autodetect_baseline_rtt_handle() builder method.
- A matched RTT sample always updates min_rtt_ms in the same
  handle_response call that returns it, so the store site reads the new
  getter unconditionally right after storing the RTT sample, with no
  before/after comparison needed (unlike the bandwidth case in #1734,
  where the underlying value can be cleared to None on an unusable
  measurement).
- Adds a manager-level test pinning the session-low-not-window-low
  property on the new getter directly, plus the usual pair of
  handle-plumbing tests (sentinel default, injected-handle round trip).

## Validation

`cargo xtask check fmt/lints/tests/typos/locks` all pass, including the
3 new tests.

## Notes

No public API break: `RdpServer::new` is crate-private, and the public
surface (`RdpServerBuilder`) only gains an additive optional field and a
new method.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-reviewed/1 One automated review completed breaking-change Includes a breaking change, and requires special scrutiny at the boundaries kind/protocol Affects RDP or related protocol behavior maintainer-required Maintainer review or intervention is required risk/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny size/M Size: up to 449 counted lines and 10 files; exceeds S in either measure

Development

Successfully merging this pull request may close these issues.

1 participant