Conversation
An embedder had no way to tell whether a session's dynamic channels, the
graphics pipeline among them, actually moved onto the reliable RDP-UDP
tunnel or stayed on TCP, nor which RDP-UDP version the tunnel negotiated.
Both depend on the server, and a failed UDP bootstrap falls back to TCP on
its own.
- rdpeudp: `RdpeudpConnection::negotiated_version` returns the version the
handshake settled on.
- rdpeudp-tokio: the driver records it when the connection is established,
and `UdpTransport::negotiated_version` exposes it.
- client: a new `RdpOutputEvent::Transport { reliable_udp, udp_version }`
(with the `udp` feature) is sent once the session is active and again
whenever either value changes: a tunnel comes up, Soft-Sync moves the
channels onto it, or the session falls back to TCP.
The viewer logs the event.
GitHub Actions
Error
github-actions
Bot
added
needs-review
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Observability-only PR: it exposes the negotiated RDP-UDP version (RdpeudpConnection::negotiated_version -> driver/SharedIo -> UdpTransport::negotiated_version) and adds a change-deduplicated RdpOutputEvent::Transport { reliable_udp, udp_version } emitted from the active-session loop, logged by the viewer. I independently verified the version mapping against the handshake state machine (client validates the SYN+ACK version against {V1,V2,V3} and the offer; server always settles V2->V3), the dedup logic (per-session state, first iteration always announces, reconnects re-announce), emission placement covering tunnel-up/Soft-Sync/TCP-fallback, feature gating, and in-tree matchers (daemon wildcard arm, viewer arm) still compiling. No correctness, protocol, or API defects found. Two low-severity log-duplication issues remain: the transport state is logged twice in-process, and the driver's new handshake-complete debug duplicates the existing one in the connect/accept flows.
| RdpOutputEvent::Transport { | ||
| reliable_udp, | ||
| udp_version, | ||
| } => { | ||
| info!(reliable_udp, ?udp_version, "Session transport"); | ||
| } |
There was a problem hiding this comment.
[skeptical + code-compressor] Transport state logged twice at info for the in-tree viewer — low 🟡 — The client logs info!(reliable_udp, ?udp_version, "Session transport") when it emits the new event (crates/ironrdp-client/src/rdp.rs:4272, added by this PR), and the viewer's new match arm logs the identical fields with the identical message on receipt (crates/ironrdp-viewer/src/app.rs:623-628, also added here). The viewer runs the client in-process, so every transport change produces two consecutive equivalent info lines in one process, in a PR whose purpose is log-based diagnostics. Any other embedder already gets the library-side line plus the event payload. Exactly one of the two logs is needed: either shrink the viewer arm to an empty match like the WindowingOrders arm above it, or drop the client-side info! in favor of embedder-owned presentation. Diagnostics-only impact, hence low severity.
There was a problem hiding this comment.
Fixed in 723ae4f. The client no longer logs the transport itself. Embedders log the event as they do the other session events, so the viewer arm keeps the single info line.
| let version = self.conn.negotiated_version(); | ||
| if let Ok(mut shared) = self.shared.lock() { | ||
| shared.negotiated_version = version; | ||
| } | ||
| tracing::debug!(?version, "RDP-UDP handshake complete"); |
There was a problem hiding this comment.
[code-compressor] Driver handshake-complete debug duplicates the connect/accept flow's existing one — low 🟡 — The added debug!(?version, "RDP-UDP handshake complete") at driver.rs:323 fires when the driver processes Event::Connected; the connect/accept flows awaiting connected_notify then log their own handshake-complete debug in the same instant (crates/ironrdp-rdpeudp-tokio/src/transport.rs:457 "RDPEUDP2 handshake complete, starting TLS" and transport.rs:653 server variant), so every successful handshake logs completion twice at debug level. Only the SharedIo write of negotiated_version is load-bearing; dropping the log line (inlining the accessor call into the field assignment) removes the noise while the version stays available via UdpTransport::negotiated_version and the client's info-level Transport event. Tradeoff: harnesses driving the Driver directly lose a same-instant debug record.
There was a problem hiding this comment.
Fixed in 723ae4f. The debug line is gone, and the negotiated version is written straight into SharedIo.
The client logged the transport when it emitted the event and the viewer logged it again on receipt. Leave logging to the embedder, as for the other session events, and drop the driver's handshake log, which repeated the one in the connect and accept flows.
github-actions
Bot
added
needs-review
|
This pull request may overlap with #2013. Both touch client-side RDP-UDP version handling. PR 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). |
|
On the overlap notice: #2013 and #2015 are complementary and merge cleanly in either order. #2013 sets the RDP-UDP version the client offers and whether it advertises the graphics pipeline. #2015 reports what the session actually ended up with: the negotiated version and whether the channels run over the reliable UDP tunnel. |
There was a problem hiding this comment.
Adds observability only: RdpeudpConnection::negotiated_version derives the settled version from NegotiatedParams.wire (V1 carries 0x0001/0x0002, V2 maps to V3), the tokio driver records it into SharedIo on Event::Connected before notifying waiters, UdpTransport exposes it, and the client emits a deduped RdpOutputEvent::Transport whenever the (reliable_udp, udp_version) tuple changes in the active loop; the viewer logs it and tests assert the version. I verified the version mapping against MS-RDPEUDP/MS-RDPEUDP2 semantics and that emission covers session start, tunnel-up, Soft-Sync, and TCP fallback; (true, None) is unreachable. Two low-severity doc issues remain: reliable_udp overclaims the graphics pipeline moved (backing state is 'any DVC'), and negotiated_version's 'once complete' timing fails on the server accept path where params are set in SynReceived.
| /// The dynamic channels, the graphics pipeline among them, travel over the reliable | ||
| /// RDP-UDP tunnel. | ||
| reliable_udp: bool, |
There was a problem hiding this comment.
[skeptical] reliable_udp doc overclaims that the graphics pipeline moved onto UDP — low 🟡 — The flag is read from ActiveStage::reliable_udp_dvc_tunnel_in_use, which is documented as 'whether Soft-Sync moved any DVC to the reliable UDP tunnel' and is implemented via has_channels_on_tunnel, an any-channel check. With declined channels producing partial moves (handled by PR #2007), some DVCs can stay on TCP, potentially including the graphics pipeline. The doc asserts they all travel over UDP, so an embedder keying diagnostics or status off reliable_udp can report graphics-on-UDP when it is not. Narrow the wording to 'at least one dynamic channel' or expose graphics-channel-specific state.
| /// The protocol version the handshake settled on, once it is complete: version 1 or 2 | ||
| /// for MS-RDPEUDP, version 3 for MS-RDPEUDP2. | ||
| pub fn negotiated_version(&self) -> Option<UdpVersion> { | ||
| self.params.as_ref().map(|params| match params.wire { | ||
| WireFormat::V1 { version } => UdpVersion(version), | ||
| WireFormat::V2 => UdpVersion::V3, | ||
| }) | ||
| } |
There was a problem hiding this comment.
[skeptical] negotiated_version returns Some before the handshake completes on the server path — low 🟡 — The doc states the value is available 'once it is complete', but accept() populates params with WireFormat::V2 while still in SynReceived, so a server-side connection reports Some(UdpVersion::V3) before the final ACK establishes the session. A caller using Some as an establishment signal is misled. In-tree use is unaffected because the tokio driver records the version only on Event::Connected, and the sibling mtu() accessor shares this early-params timing, but this is new public API text whose stated contract the implementation violates on the public accept path. Fix the doc or gate on the Established state.
github-actions
Bot
added
ai-reviewed/2
…#2007) Windows lists every dynamic channel it intends to move in its Soft-Sync request, including the ones the client declined with NO_LISTENER. Against a Windows 11 host the request lists channels 2, 6, 7, 8, 9, 10, 11 and 12 (CoreInput, MouseCursor, Graphics, Video, Geometry, ...), and only channel 7, the graphics pipeline, is open. `process_soft_sync_request` dropped a whole channel list as soon as one ID in it was not open. The tunnel was then never switched, and the channels the client had opened stayed on TCP while the server was already sending them on the tunnel (MS-RDPEDYC 3.2.5.3.1). Unopened channels are now skipped one by one, and the tunnel is switched for the rest. ## Testing - New `dvc::client::soft_sync_skips_channels_the_client_did_not_open` in `ironrdp-testsuite-core`. - Live, against a Windows 11 host over RDP-UDP version 2, with the viewer built from a branch that also carries the tunnel and client PRs of this series: the Soft-Sync request above now switches the tunnel, and the graphics pipeline moves onto it. ## Checks - `cargo fmt --all -- --check` - `cargo clippy --workspace --all-targets --features helper,__bench --locked -- -D warnings` - `cargo test --locked -p ironrdp-testsuite-core -p ironrdp-testsuite-extra`, plus the lib tests of the crates touched here - `cargo test --workspace --locked` on a branch that merges this PR with the other Windows interop PRs from this series - `typos` on the changed files ## Series These PRs port the Windows interop fixes and Linux backends from a downstream IronRDP fork, so the fork can be retired. Each one is based on `master` and can be reviewed and merged on its own. I also checked that all of them merge cleanly together in this order. - #2007 fix(dvc): Soft-Sync tunnel with declined channels - #2008 fix(session)!: channels and graphics on the tunnel - #2009 fix(rdpeudp): auto-detect on the tunnel - #2010 fix(graphics)!: SRL streams from Windows - #2011 fix(egfx): bitmap cache across ResetGraphics - #2012 feat(session): bandwidth measurements during the session - #2013 feat(client): graphics pipeline and RDP-UDP version options - #2014 fix(client): resize reconnects on the graphics pipeline - #2015 feat(client): transport event - #2016 feat(cliprdr): Linux clipboard backend - #2017 feat(rdpdr): printer on Linux and macOS Co-authored-by: AKolenda <testedemail2222@gmail.com>
|
Closing this one. It only reports the negotiated transport and RDP-UDP version to embedders, and nothing else in the series depends on it. |
An embedder had no way to tell whether a session's dynamic channels, the graphics pipeline among them, actually moved onto the reliable RDP-UDP tunnel or stayed on TCP. It also could not tell which RDP-UDP version the tunnel negotiated. Both depend on the server, and a failed UDP bootstrap falls back to TCP on its own.
RdpeudpConnection::negotiated_versionreturns the version the handshake settled on, as aUdpVersion.UdpTransport::negotiated_versionexposes it.RdpOutputEvent::Transport { reliable_udp, udp_version }(with theudpfeature) is sent once the session is active. It is sent again whenever either value changes: a tunnel comes up, Soft-Sync moves the channels onto it, or the session falls back to TCP.The viewer logs the event.
RdpOutputEventgains a variant; the other in-tree matches (daemon, ActiveX) already have a wildcard arm.Testing
full_handshake_client_serveranda_client_follows_a_syn_ack_that_settles_on_version_2now also assert the negotiated version.reliable_udp=false udp_version=Some(UdpVersion(2))once the session is active, thenreliable_udp=trueright after Soft-Sync.Checks
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --features helper,__bench --locked -- -D warningscargo test --locked -p ironrdp-testsuite-core -p ironrdp-testsuite-extra, plus the lib tests of the crates touched herecargo test --workspace --lockedon a branch that merges this PR with the other Windows interop PRs from this seriestyposon the changed filesSeries
These PRs port the Windows interop fixes and Linux backends from a downstream IronRDP fork, so the fork can be retired. Each one is based on
masterand can be reviewed and merged on its own. I also checked that all of them merge cleanly together in this order.