feat(server): measure bandwidth and report it in NetworkCharacteristicsResult - #1471
Conversation
3643fdb to
61ddd95
Compare
github-actions
Bot
added
scope/core
61ddd95 to
463ba3c
Compare
github-actions
Bot
added
size/L
463ba3c to
aaa85dd
Compare
github-actions
Bot
added
size/L
Benoît Cortier (CBenoit)
added
ai-reviewed/1
|
Review found blocking issues: continuous bandwidth detection incorrectly uses connect-time-only BW_PAYLOAD handling, and NetworkCharacteristicsSync state is ignored. Additional findings include sequence-wrap correlation risk, oversized malformed public PDU encodings, and stale MCS transport documentation; the security-header concern is not applicable. |
## Summary - `AutoDetectManager` read the clock itself: `std::time::Instant` in `pending_probes`, `Instant::now()` in `send_rtt_request`, and `Instant::elapsed()` in `handle_response` and `expire_stale_probes`. - It now takes `now_ms`, a caller-supplied monotonic millisecond counter whose epoch is arbitrary as long as it's consistent across calls. `ironrdp-server` supplies it from a process-wide monotonic origin in `server.rs`, so the clock lives in the I/O driver rather than in the state machine. ## Why - **Testability.** The RTT assertions were wall-clock dependent. `snapshot_reflects_measurements` could only check that the average came out under an arbitrary 100 ms bound, which almost any bug would satisfy. It now supplies both timestamps and asserts exact values: samples of 10, 20 and 30 ms giving min 10, max 30, average 20. - **Portability.** `std::time::Instant::now` panics on `wasm32-unknown-unknown`, so a type that reads it internally can't be reused from a WASM build. - **Layering.** A state machine that reads ambient time can't satisfy the no-I/O rule the Core Tier crates follow, which forecloses moving this code in that direction later. Also covers the edges of the arithmetic the injected clock exposes. There are two `saturating_sub` sites and they saturate in opposite directions: - `handle_response`: a clock that ran backwards between request and response yields a zero sample rather than a wrapped value near `u32::MAX`, and the zero reaches the sample window, not just the return value. - `expire_stale_probes`: the same backwards clock makes the age zero, which is below any maximum, so the probe stays pending. Wrapping would make it look older than any limit and drop a probe whose response is still in flight. A third test covers the `u32::try_from(..).unwrap_or(u32::MAX)` on the first of those lines, where a gap wider than about 49.7 days clamps rather than truncating to the low 32 bits. ## Validation `cargo xtask check fmt/lints/tests/typos/locks` all pass. Each of the three new tests was checked against a mutation of the code it guards rather than only for passing: the two backwards-clock tests fail if either `saturating_sub` becomes `wrapping_sub`, and the clamp test fails if the `try_from` becomes a truncating `as u32`, which reports `Some(0)` for a 49.7 day gap. ## Notes - Came out of the discussion on #1465, where the same question arises on the connector side. `ironrdp-connector` reads no clock at all today, and answering a connect-time Bandwidth Measure properly needs one; taking the timestamp from the caller is the shape that works on every target. - `AutoDetectManager` arrived in #1177 with the internal clock, so this corrects code I wrote rather than anyone else's. BREAKING CHANGE: `AutoDetectManager::send_rtt_request` now takes `now_ms`; `handle_response` takes `now_ms`; `expire_stale_probes` takes `now_ms` and a `max_age_ms` `u64` instead of a `core::time::Duration`. The `RTT_PROBE_MAX_AGE` constant is now `RTT_PROBE_MAX_AGE_MS`, a `u64` of milliseconds. ## Two PRs are stacked on this #1470 and #1471 are built on this branch and cannot merge before it. The merge order is this PR, then #1470, then #1471. This is also where the stack's breaking-change marker lives. The `!` here covers the arity changes to `AutoDetectManager::send_rtt_request` and `handle_response`; `cargo semver-checks` attributes both to this PR and reports no further update required for either of the two above when baselined against it. ## Rebased Rebased onto `master` on 2026-08-02 as one `--update-refs` operation with the rest of the stack, so the checks run against the current tree rather than the state before that day's merges. No conflicts, no content change. All five gates green on this head independently, not only at the stack tip.
The server-side AutoDetectManager measures RTT but never reports the result back to the client, so clients cannot use the measurement to size their receive buffers. Add a NETCHAR_RESULT_RTT emission on the MCS message channel: a netchar_result_rtt() constructor (baseRTT + averageRTT, no bandwidth), an AutoDetectManager::build_netchar_result() that reports the lowest and average observed RTT once samples exist, and a send site on the existing auto-detect tick that reuses the message-channel framing from the RTT request path. Bandwidth is intentionally omitted here; a follow-up adds continuous bandwidth measurement and upgrades the result to NETCHAR_RESULT_ALL. Also corrects a stale doc comment on send_rtt_request that referred to the IO channel Share Data PDU (auto-detect moved to the MCS message channel).
…csResult Builds on the RTT-only Network Characteristics Result: the server now runs a continuous Bandwidth Measure over the MCS message channel and folds the result into the report, so a participating client (mstsc, RDM, FreeRDP) receives the full baseRTT + bandwidth + averageRTT characteristics rather than RTT alone. Once per BW_MEASURE_INTERVAL_TICKS auto-detect ticks the AutoDetectManager emits a Bandwidth Measure transaction (Start -> Payload -> Stop, request type 0x0014 / 0x0429) back-to-back so the client counts only the payload window; its Bandwidth Measure Results reply (timeDelta + byteCount) is turned into a kbps figure that upgrades the emitted result to the all-fields form. A measurement is never started while one is outstanding. Reuses the existing message-channel framing; no new PDU types.
aaa85dd to
c99a8e8
Compare
github-actions
Bot
added
size/M
github-actions
Bot
added
maintainer-required
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The Network Characteristics Result half is sound: netchar_result_rtt is correctly shaped for 0x0840, the encoder derives field presence from requestType, and the fixture plus round-trip case pin it. The bandwidth half is not ready. It injects a Bandwidth Measure Payload into a continuous window that this crate's own docs and MS-RDPBCGR 2.2.14.1.3 both scope to connect-time, risking a doubled byteCount or client rejection; and it opens and closes the window in three back-to-back writes, so timeDelta is timer quantization noise and the resulting kbps figure, published as the current bandwidth, is meaningless. pending_bw also has no expiry counterpart to the existing expire_stale_probes, so one unanswered measurement disables the feature for the session. Cleanest path: land the RTT-only reporting as-is and take the bandwidth transaction separately, emitting Start/Stop around real traffic over a bounded interval with expiry and staleness handling.
Protocol analysis: partially_accepted — Confirmed: netchar_result_rtt conforms (netchar_fields(0x0840), headerLength 0x0E, 14-byte fixture), the 0x0014/0x0429 codes with payload None, the (byteCount*8)/timeDelta formula and zero guard, and the send_rtt_request doc fix. Escalated: the connect-time-only Bandwidth Measure Payload is the strongest defect, provable from this crate's own module doc and BW_PAYLOAD annotation, not only the spec. Refined: never-expiring pending_bw is graded ambiguous, but against the repo's existing expire_stale_probes precedent it is a concrete defect, reported as blocking. Rejected: the three PDUs are not written without an intervening await — each write_all awaits and SharedWriter locks per call (server.rs:2200), so the display task can interleave writes between Start and Stop.
- blocking / medium — crates/ironrdp-server/src/autodetect.rs
pending_bw gates every future measurement but is cleared only by a Bandwidth Measure Results carrying the exact sequence number; there is no expiry counterpart to expire_stale_probes, which this file already provides for the RTT path because the identical hazard was recognized there. The client reply is not guaranteed (it may be lost, or withheld if the client rejects the out-of-phase payload), so a single unanswered measurement makes build_bandwidth_measure return None for the remainder of the connection and silently disables the feature this PR adds. Ageing pending_bw out on the same now_ms clock the RTT probes use would close the gap and keep the two paths symmetric. - non_blocking / low — crates/ironrdp-server/src/server.rs
handle_response returns Option<u32> for both "this was an RTT sample" and "nothing was recorded", so a Bandwidth Measure Results that is accepted and stored now takes the else branch and logs "Unmatched auto-detect response". The one observable signal that the new bandwidth path is working reports it as a failure. Returning a small enum, or logging the bandwidth explicitly, would keep the diagnostics truthful.
| AutoDetectRequest::bw_start_continuous(seq), | ||
| AutoDetectRequest::bw_payload(seq, vec![0u8; BW_PAYLOAD_LEN]), | ||
| AutoDetectRequest::bw_stop_continuous(seq), | ||
| ]) |
There was a problem hiding this comment.
blocking / critical: The continuous transaction injects a Bandwidth Measure Payload (requestType 0x0002) between a 0x0014 Start and a 0x0429 Stop. This crate's own documentation already declares that message connect-time only: the module header of crates/ironrdp-pdu/src/rdp/autodetect.rs states that during continuous detection actual PDU traffic between BW_START and BW_STOP replaces the payload messages, the BW_PAYLOAD constant is annotated "connect-time only", and so is the BandwidthMeasurePayload variant. MS-RDPBCGR 2.2.14.1.3 scopes it the same way. A client applying the 0x0014 rule (count all data between Start and Stop) plus the explicit payload rule (add payloadLength + 8) double-counts the same 8200 bytes, roughly doubling the reported byteCount and the bandwidth the server then publishes; a stricter client may reject the out-of-phase message and never reply, which permanently stalls the measurement (see the pending_bw finding). Separately the payload is vec![0u8; 8192] while bw_payload's own doc comment says "random data". The transaction should be Start then Stop, with ordinary server output filling the window.
| let data = encode_autodetect_request(pdu, message_channel_id, user_channel_id)?; | ||
| writer.write_all(&data).await?; | ||
| } | ||
| } |
There was a problem hiding this comment.
blocking / high: The measurement window is never paced: Start, Payload and Stop are encoded and written in one tight loop, so the client's Network Characteristics Timer measures an interval of roughly zero. With time_delta_ms == 0 computed_bandwidth_kbps returns None and the sample is silently dropped; with 1 ms it yields 8200 * 8 / 1 = 65600 kbps, a figure produced entirely by timer quantization and local socket-buffer speed rather than link capacity. That number is then published to the client in the 0x08C0 result as the current bandwidth, which the comment itself says the client uses to size receive buffers. The accompanying comment ("sent back-to-back so the client counts only the payload window") is also incorrect: SharedWriter::write_all (server.rs:2200) acquires the mutex per call, so the display task holding its own clone can interleave an arbitrary amount of frame data between the Start and the Stop writes, making byteCount nondeterministic. A meaningful measurement needs the window held open across real traffic for a bounded duration, not three writes in a row.
| AutoDetectRequest::netchar_result(seq, snapshot.min_ms, bandwidth_kbps, snapshot.avg_ms) | ||
| } | ||
| None => AutoDetectRequest::netchar_result_rtt(seq, snapshot.min_ms, snapshot.avg_ms), | ||
| }) |
There was a problem hiding this comment.
non_blocking / medium: Once set, bandwidth_kbps is retained for the life of the manager and every subsequent result is emitted in the 0x08C0 all-fields form. MS-RDPBCGR 2.2.14.1.5 defines that field as the current bandwidth, but combined with the non-expiring pending_bw a single early sample can be reported as current for the whole session, including after a link change. Ageing the value out (reverting to the 0x0840 RTT-only form once it is stale) would keep the reported characteristics honest; the round-trip machinery for that reversion already exists.
| } | ||
| None | ||
| } | ||
| _ => None, |
There was a problem hiding this comment.
non_blocking / medium: A Network Characteristics Sync from the client falls into the catch-all arm and is discarded. MS-RDPBCGR 3.3.5.14 requires the server to stop any RTT or bandwidth measurement in progress and adopt the transmitted bandwidth and rtt values. The omission predates this change, but this PR introduces exactly the bandwidth_kbps and pending_bw state the rule targets, so ignoring the message is now materially wrong rather than merely incomplete. Reasonable as a follow-up rather than a condition on this PR.
| if let Some(p) = mgr.build_bandwidth_measure() { | ||
| break p; | ||
| } | ||
| }; |
There was a problem hiding this comment.
non_blocking / low: Both new bandwidth tests drive the pacing with an unbounded `loop { if let Some(p) = mgr.build_bandwidth_measure() { break p } }` (mirrored at crates/ironrdp-server/src/autodetect.rs:396). If a future change makes the measurement never become due — for instance an expiry or pacing regression — these tests hang the test run instead of failing, which is a much worse CI signal than an assertion. Bounding the loop to a small multiple of BW_MEASURE_INTERVAL_TICKS and asserting it fired would fail loudly instead.
2 participants
Stacked on #1470, which is itself stacked on #1487, so the merge order is #1487, then #1470, then this. This branch carries both ancestors' commits, so it builds and tests green on its own. The breaking signature changes visible against master come from #1487; baselined against it, this PR reports no semver update required.
Extends the server-side auto-detect added by #1470. The RTT-only Network Characteristics Result becomes the full baseRTT plus bandwidth plus averageRTT report by adding a continuous Bandwidth Measure over the MCS message channel.
Change: once per N auto-detect ticks, AutoDetectManager emits a Bandwidth Measure transaction (Start, Payload, Stop; continuous request type 0x0014 / 0x0429) back to back on the message channel. The client counts the payload window and replies with a Bandwidth Measure Results PDU (timeDelta plus byteCount); the manager turns that into a kbps figure and upgrades the next Network Characteristics Result from the RTT-only form to the all-fields form (NETCHAR_RESULT_ALL). A new measurement is never started while one is outstanding. It reuses the message-channel framing from #1348 and adds no new PDU types.
Testing: unit and integration tests cover pacing, the Start/Payload/Stop transaction sharing one sequence number, bandwidth-results handling, and the RTT-only to all-fields upgrade.
cargo xtask check fmt/lints/tests/typos/locksall pass.Rebased
Rebased onto
masteron 2026-08-02 as one--update-refsoperation with the rest of the stack, so the checks run against the current tree rather than the state before that day's merges. No conflicts, no content change. All five gates green on this head independently, not only at the stack tip.