feat(server): measure bandwidth on the UDP tunnel when EGFX uses it

[Greg Lamberson (glamberson)]

Summary

• Once EGFX moves to the UDP transport, no large write crosses TCP any more, so the bracketed bandwidth measurement never runs again and the reported figure stays at its pre-migration value for the rest of the session.
• Large EGFX batches on the tunnel are now bracketed the same way, with Bandwidth Measure Start and Stop (0x0014 / 0x0429) in RDP_TUNNEL_SUBHEADERs (MS-RDPBCGR 1.3.9, 2.2.14.1.2). Each sub-header is the request itself: Its SubHeaderLength and SubHeaderType are the request's headerLength and headerTypeId (MS-RDPEMT 2.2.1.1.1), so the request's first two bytes are not repeated in SubHeaderData. Each goes in a Tunnel Data PDU with no data of its own, so the client's count (only data after the tunnel header, 3.2.5.14) is exactly the graphics between them.
• Bandwidth Measure Results the client returns in tunnel sub-headers are handled like the ones on the message channel; they are decoded from the whole sub-header, which is the response structure; the shared handling moved into one method.

Validation

cargo xtask check fmt/lints/tests/typos/locks all pass. Tests in ironrdp-testsuite-core cover the sub-header encoding of Start and the decoding of results arriving in tunnel sub-headers into the auto-detect manager, through a private __test feature on ironrdp-server that exposes those two functions, as well as the sub-header wire layout against the spec.

Notes

Stacked on #2021 (bracketed measurement), whose commits come first in this branch; this PR's own changes are the commits from feat(server): measure bandwidth on the UDP tunnel when EGFX uses it onward. #1954 and #2030 have merged.

[github-actions]

This pull request may overlap with #2021.

This PR contains the same server-side feature #2021 describes: bracketing large EGFX writes with Bandwidth Measure Start/Stop in ironrdp-server's autodetect.rs and server.rs, with the same 10 KiB threshold, once-per-second pacing, tick-window fallback, and 30 s expiry, plus the same tests/server/autodetect.rs tests. The head SHA differs from #2021's, so the overlap is shared scope for a human to assess, not a verdict of redundancy.

This notice is advisory only. Automated review continues as usual, and how these pull requests relate is for maintainers and authors to decide.

Note

LLM-assisted content (no human feedback).

[AKolenda]

For context, #2009 is the client side of this. It answers the RTT and bandwidth requests Windows sends in tunnel sub-headers, and it reads a sub-header as the auto-detect structure itself, the same way your latest commit does.

I ran your Start (06 00 07 00 14 00) through the #2009 responder and decoded its Results the way record_tunnel_sub_headers does, and they line up, so an IronRDP client and server should agree on the tunnel.

The two branches do conflict in ironrdp-rdpeudp-tokio (transport.rs, tunnel.rs, framed.rs), because #2009 adds its own way to push encoded PDUs through the write pump. TunnelMessage from #2030 is the cleaner way to do that, so if #2030 lands first I'll rebase #2009 onto it and drop the Outgoing enum.

[github-actions]

Independent review of PR #2031 (bandwidth measurement bracketed around large EGFX writes, with Start/Stop/Results carried as RDP_TUNNEL_SUBHEADERs over the reliable UDP tunnel). The change is sound: the manager state machine (Bracketing/AwaitingResults states, pacing, expiry of stalled measurements), the tunnel sub-header encode/decode helpers, and the shared record_autodetect_response path are coherent, protocol-consistent, and tested. All four validated specialist candidates were verified against the head tree and confirmed: the local bandwidth computation diverges from the now-orphaned PDU helper's zero-timeDelta semantics, a redundant bool remains in the EGFX routing code, the testsuite mirrors BW_BRACKET_MIN_BYTES instead of importing it via the PR's own __test mechanism, and the AwaitingResults since_ms Option exists only because build_bandwidth_measure cannot see the clock its caller has. No correctness defects were found independently; the four published findings are low-to-me…

1. [skeptical] measured_bandwidth_kbps diverges from the orphaned pdu computed_bandwidth_kbps helper — medium 🟠 — crates/ironrdp-server/src/autodetect.rs
   The PR replaces the call to AutoDetectResponse::computed_bandwidth_kbps with a local fn whose semantics differ: timeDelta 0 is clamped to 1 ms instead of yielding None. ironrdp-pdu still ships computed_bandwidth_kbps (documented 'None if timeDelta is zero', with tests pinning that behavior), and after this PR nothing in the workspace calls it outside its own tests. The workspace therefore carries two divergent definitions of the same derivation, and the deliberate semantic change (a real, bounded measurement vs. a discarded one) is made without touching the helper's docs/tests. Move the changed semantics into computed_bandwidth_kbps (updating its tests and noting the API change) or justify in code why the server must diverge.
2. [code-compressor] route_over_udp bool is now redundant with udp_route — low 🟡 — crates/ironrdp-server/src/server.rs
   The refactor leaves route_over_udp read exactly once, only to set udp_route = Some(udp_transport); udp_route alone drives the final dispatch. The intermediate bool can be deleted by folding the tunnel_for_outgoing_channel comparison directly into the if, removing a binding that carries the same fact as udp_route. No behavior change; purely local polish.
3. [code-compressor] AwaitingResults::since_ms Option exists only because build_bandwidth_measure takes no clock — low 🟡 — crates/ironrdp-server/src/autodetect.rs
   The tick-window path sets since_ms: None at Stop time, and expire_stale_probes carries a branch that stamps it on the first expiry pass that sees it. That lazy-stamp state (an Option, a mutation inside a match on &mut self.pending_bw, a documented subtlety, and a dedicated two-pass test) exists solely because build_bandwidth_measure cannot know the current time, while its single production caller already has monotonic_now_ms available. Taking now_ms and stamping at Stop-send removes the Option, the branch, and the test, at the cost of a public-signature change and a slightly tighter expiry window. Current form is correct; optional compression.

[Greg Lamberson (glamberson)]

Fair point, and the two functions differ in more than the zero time. This one came in with #2021's first commit, which replaced the server's call to computed_bandwidth_kbps, so I put the explanation next to the divergence. The doc comment on measured_bandwidth_kbps now says why it doesn't use the ironrdp-pdu helper. That helper returns None for a zero time, which would turn the sub-millisecond bursts a fast link produces into failed measurements that clear the figure, and it returns Some(0) for zero bytes, where a measurement that counted nothing should clear the figure. I left the helper alone, since changing its documented, tested behavior would change a published crate's API. The change is comment-only and goes in with the next push.

[Greg Lamberson (glamberson)]

Right, that binding was left over from the routing change. On master the flag was read again at the end of the function, and with udp_route doing that job it only fed the one if. I folded the tunnel comparison into the if and removed the local. Behavior is unchanged, and egfx_moves_onto_the_udp_tunnel_with_soft_sync fails if the condition is forced off. The change goes in with the next push.

[Greg Lamberson (glamberson)]

Thanks, that's a fair simplification, and the production caller does have a clock. I'm leaving it, because build_bandwidth_measure is a public method on master with no clock parameter, and taking one would be a breaking change to a published crate just to drop an internal Option. The lazy stamp costs at most one tick of extra expiry slack, it's documented on the variant, and an_unanswered_measurement_expires fails if the stamp is removed.

[github-actions]

PR 2031 brackets large EGFX writes with Bandwidth Measure Start/Stop on both TCP and the reliable UDP tunnel, carrying auto-detect requests/results as RDP_TUNNEL_SUBHEADERs whose two header bytes double as the auto-detect header, and adds expiry for stalled measurements. Verified against the head: the lifecycle is closed on both paths and backstopped by expire_stale_probes; the sub-header wire form matches MS-RDPEMT 2.2.1.1.1 and is pinned by tests; responses are handled from either transport. Of the two candidates, the skeptical formula-divergence finding is valid in root cause but factually wrong that the divergence is unenforced: this PR's own tests fail if the call reverts to the ironrdp-pdu helper, so only the overflow-policy divergence remains unpinned (refined). The code-compressor inline suggestion is a neutral style tradeoff whose own rationale concedes keeping the documented wrapper is reasonable; it is rejected.

1. [skeptical] Bandwidth formula diverges from ironrdp-pdu helper by design; divergence is test-pinned, only overflow policy is not — low 🟡 — crates/ironrdp-server/src/autodetect.rs
   measured_bandwidth_kbps reimplements AutoDetectResponse::computed_bandwidth_kbps with deliberate zero-case differences: time_delta_ms is floored to 1 ms instead of failing, and a zero byte_count clears the figure instead of reporting Some(0). Unlike the original candidate claimed, this is enforced rather than only explained: zero_time_delta_counts_as_one_millisecond and zero_byte_count_ages_out_a_previous_bandwidth_figure (both in this PR) fail if the call is swapped back to the helper, so a revert cannot silently regress fast-link or empty measurements. The residual, unpinned divergence is overflow behavior: the helper truncates with `as u32` while measured_bandwidth_kbps saturates at u32::MAX; a test at the u32::MAX boundary would pin that too. Documentation-only follow-up; no correctness impact.

[Greg Lamberson (glamberson)]

Good catch, the overflow policy was the one difference nothing pinned. I added an_oversized_bandwidth_figure_saturates, which feeds a 1 ms result with u32::MAX bytes and expects u32::MAX, and it fails if the conversion truncates instead. The doc comment on measured_bandwidth_kbps now says the figure saturates where the helper wraps, because the client supplies both numbers, and that u32::MAX also means not measured in the exposed bandwidth handle. The change goes in with the next push.