feat(server): wire UDP multitransport into ironrdp-server - #1954
Open
Greg Lamberson (glamberson) wants to merge 9 commits into
Open
Greg Lamberson (glamberson) wants to merge 9 commits into
Greg Lamberson (glamberson) wants to merge 9 commits into
Conversation
Add a MultitransportBootstrapping state to the acceptor sequence, entered right after licensing. It advertises UDP multitransport support in the GCC Server MultiTransportChannelData block when configured (set_multitransport_offer), and, once the client reciprocates the reliable-UDP flag, sends the Initiate Multitransport Request (MS-RDPBCGR 2.2.15.1) on the MCS message channel before moving straight on to capability negotiation. The acceptor does not wait for the client's Initiate Multitransport Response before continuing: MS-RDPBCGR 3.2.5.15.1 only obliges the client to send one when Soft-Sync is negotiated or the sideband attempt failed, so blocking on it would stall the handshake on the common successful path. multitransport_request() surfaces the sent request so the caller can establish the sideband UDP transport in parallel. A response that does arrive lands before the mandatory Confirm Active (the client sends it, if at all, before it ever reads Demand Active), so CapabilitiesWaitConfirm recognizes and drops it by channel rather than erroring on the unexpected payload. Adds ironrdp-testsuite-core coverage for the offer/no-offer/no-client- support paths and for the response-before-Confirm-Active ordering.
Greg Lamberson (glamberson)
deployed
to
llm-providers
September 10, 2026 22:08 — with 
GitHub Actions
Active
GitHub Actions
Active
github-actions
Bot
added
breaking-change
Contributor
|
Potential duplicate detected: #1951. The acceptor portion (MultitransportBootstrapping state, set_multitransport_offer, GCC advertisement, message-channel request, acceptor.rs tests) matches Maintainer review is required. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
The acceptor-level multitransport work is sound (state machine, message-channel gating, GCC advertisement, response tolerance, tests), but the runtime integration has three significant defects: EGFX Soft-Sync is initiated without advertising SOFTSYNC_TCP_TO_UDP or awaiting a successful Initiate Multitransport Response (violates MS-RDPEDYC/MS-RDPBCGR), UdpTransportHandle::recv holds the tokio Mutex across the idle await and can deadlock sends, and the finalize handler awaits the full 15s UDP accept inline, stalling the handshake. Also published: the routing gate treats a declining SoftSyncResponse as acceptance, the false one-connection-at-a-time premise under preemption, unconditional UDP dependencies for an opt-in feature, and two minor cleanup/test-duplication items. Two optional compression proposals (defensive finalize guard, folding the bootstrapping state) were rejected.
github-actions
Bot
added
ai-reviewed/1
The Server MultiTransportChannelData block was advertised whenever the server's own offer was configured, regardless of whether the client actually populated its own Client MultiTransportChannelData block. MS-RDPBCGR 2.2.1.4 requires the server block to be omitted when the client did not send one. Added a client_offered_multitransport field tracking presence separately from multitransport_flags, which alone can't distinguish "absent" from "present but empty", and gated the offer on it. The Initiate Multitransport Response has no fixed position relative to the rest of the handshake (3.2.5.15.1): a conforming client can send it after Confirm Active, during ConnectionFinalization, not only before it. None of FinalizationSequence's own PDU decoders expect it, so depending which sub-state was active a late response was silently swallowed while advancing a state, propagated as a connection-ending decode error, or surfaced to the embedding application as a raw input event. Added the same tolerance CapabilitiesWaitConfirm already had to ConnectionFinalization. The late-response guard itself only checked channel and outstanding- request, not whether the payload actually decoded as a response. Since the message channel also carries Auto-Detect Response and Heartbeat PDUs (2.2.1.4.5, 2.2.8.1.1.2.1), that traffic was misclassified and dropped instead of falling through to its own handling. The guard now requires a successful strict decode, mirroring how ClientConnectorState::ConnectTimeAutoDetection demuxes the same channel client-side. Also: corrected the multitransport_request() doc, which claimed it returns None on reactivation when the carried-forward request in fact keeps it Some (intentional, needed for the late-response guard to keep working across reactivation); simplified an Option<u32> round-trip in the response-logging path down to a direct comparison, since the calling guard already guarantees a request is outstanding; and fixed two test cases constructing an S_OK response without the server advertising Soft-Sync, which 2.2.15.2 disallows. Regression tests added for the finalization tolerance and the non-response message-channel traffic case; both verified to fail against the prior behavior and pass with the fix.
…ping Documents the interim limitation of set_multitransport_offer: this acceptor only sends the request, it does not establish the sideband UDP transport itself. Marks AcceptorState non_exhaustive, matching ClientConnectorState's convention. Changes multitransport_soft_sync_negotiated to return Option<bool>, None before a request was actually sent, rather than deriving from GCC flags alone which could report true with nothing sent. Replaces the client_offered_multitransport bool with a single multitransport_flags: Option<MultiTransportFlags> field, removing the duplicated absent-vs-empty distinction. Merges log_multitransport_response into late_multitransport_response, removing the panic-prone two-step coupling, and folds the CapabilitiesWaitConfirm pre-check into the main match arm. Adds a multitransport_acceptor(offer) test factory, removing repeated setup across four tests.
Add an async driver mirroring the client side's connect_finalize_with_multitransport: it drives the acceptor sequence to completion the same way accept_finalize already does, and awaits an app-supplied handler once, synchronously, the moment the acceptor sends an Initiate Multitransport Request, so the caller can establish the sideband UDP transport (RDPEUDP2 + TLS + RDPEMT). Unlike the client-side callback, the handler reports nothing back into the sequence: the acceptor has already sent the request and moved on by the time the handler runs, so there is no response to build from an outcome. The handler should return promptly (e.g. by spawning the actual work) rather than driving the transport to completion inline, or the handshake stalls behind it. accept_finalize becomes a thin wrapper around this with a no-op handler, matching the client side's connect_finalize/connect_finalize_with_multitransport relationship. The driver seeds its "already notified" tracking from whatever request is already present rather than starting at false: a Deactivation- Reactivation Sequence rebuilds the acceptor via new_deactivation_reactivation(), which carries the original request forward without running bootstrapping again, then this function is called a second time on the rebuilt acceptor. Without the seed, that second call's first loop iteration would treat the carried-over request as newly sent and notify the handler again. Adds integration tests in ironrdp-testsuite-core driving a real Acceptor over a tokio::io::duplex pair with a hand-rolled client script: the handler fires exactly once with the sent request and does not block the handshake, a late Initiate Multitransport Response is still tolerated ahead of Confirm Active during the async-driven path, and the handler does not fire again across a reactivation round. Building the first of these surfaced a real bug: an initial take_multitransport_request() consumed the same field CapabilitiesWaitConfirm's response tolerance depends on, breaking that check the moment the driver read the request. Removed in favor of the local flag, keeping multitransport_request() a plain borrow.
Four of the eight findings were already resolved by the rebase onto Devolutions#1951's own review-response commit: the late-response tolerance now applies uniformly to every FinalizationSequence sub-state, the server MultiTransportChannelData block is filtered on the client's own block presence, and two stale doc comments were already corrected. For the remaining four, deduplicated the MCS SendDataRequest encoder and the client GCC-block builder between acceptor.rs and multitransport_finalize.rs (both made pub(super) and reused), extracted a shared play_confirm_active_and_finalization helper covering the Confirm Active plus four-PDU finalization exchange that play_client and play_reactivation_round both repeated, and extracted a recording_handler factory removing duplicated Arc::clone-into-closure plumbing across the two handler tests. play_client now also returns the request_id it decoded so the first handler test can assert the handler received the exact request the acceptor sent, rather than recording it unread. Added the missing assertion to multitransport_not_offered_by_default, whose doc comment claimed the server's GCC advertisement was checked when nothing actually was.
Greg Lamberson (glamberson)
force-pushed
the
feat/server-multitransport-wiring
branch
from
September 11, 2026 23:32
03037a4 to
5f5e9bb
Compare
Greg Lamberson (glamberson)
deployed
to
llm-providers
September 11, 2026 23:32 — with
GitHub Actions
Active
GitHub Actions
Active
github-actions
Bot
added
size/XXL
Contributor
|
This pull request may overlap with #1951. Shared scope on server-side UDP multitransport bootstrapping: a MultitransportBootstrapping acceptor state after licensing, set_multitransport_offer() gated on the client's GCC MultiTransportChannelData, the Initiate Multitransport Request on the MCS message channel, multitransport_request() surfacing it, and acceptor tests. The diff also contains the accept_finalize_with_multitransport driver PR 1953 describes. 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). |
Greg Lamberson (glamberson)
deployed
to
llm-providers
September 12, 2026 00:37 — with
GitHub Actions
Active
GitHub Actions
Active
Contributor
There was a problem hiding this comment.
PR wires opt-in UDP multitransport through the acceptor (MultitransportBootstrapping state, late-response tolerance, accept_finalize_with_multitransport), a server-side RDPEUDP2+TLS+RDPEMT accept path, and soft-sync-gated EGFX migration, all default-off behind with_udp_transport. The state machine, GCC gating, strict response decoding, and the dvc outgoing-map fix check out. Published: a high-severity silent EGFX black-hole after mid-session tunnel closure (send failures unhandled, no TCP fallback), a migration gate frozen at acceptance that late responses can never enable, an integration test whose scripted client violates the S_OK/SOFTSYNC rule, and low-severity cleanup items (unused accessor, unconditional deps, two duplications). Rejected the speculative auto-detect swallow finding: the response decoder strictly validates SEC_TRANSPORT_RSP, so mismatched message-channel traffic fails decode and falls through.
- [code-compressor] Three new hard dependencies back only an opt-in runtime path — low 🟡 — crates/ironrdp-server/Cargo.toml
ironrdp-rdpemt, ironrdp-rdpeudp, and ironrdp-rdpeudp-tokio are unconditional dependencies yet are consumed solely by src/multitransport.rs, whose runtime path only executes when udp_bind_addr is Some (None by default). Marking them optional behind a dedicated/egfx feature with #[cfg] pairs on the module and its plumbing would shrink the default build with identical runtime behavior; the author acknowledges the mechanical breadth in review.
github-actions
Bot
added
ai-reviewed/2
- Document that USER_CHANNEL_ID doubles as the fixed MCS server channel ID (MS-RDPBCGR 3.3.1.5) that every server-to-client Send Data Indication in this file relies on. - Fix the finalize integration test to send an abort response instead of S_OK when SOFTSYNC_TCP_TO_UDP was not negotiated (MS-RDPBCGR 2.2.15.2), matching the existing pattern in acceptor.rs. - Rename late_multitransport_response to is_late_multitransport_response and return bool instead of an Option<PDU> neither caller reads. - Simplify multitransport_acceptor to pass its Option straight through to set_multitransport_offer instead of re-wrapping it.
Depends on Devolutions#1953 (stacked branch, feat/acceptor-multitransport-finalize). Add RdpServerBuilder::with_udp_transport(udp_bind_addr), opt-in and None by default. When set (and the security mode is Tls or Hybrid, matching the reference client's Enhanced-Security-only gate), the acceptor offers reliable UDP multitransport, and accept_finalize uses accept_finalize_with_multitransport with a callback that binds a fresh UDP socket per connection, reuses the connection's own TLS certificate (TlsAcceptor::config()) for the sideband transport, and calls accept_udp(). Any failure at any stage falls back to TCP-only, never fails the connection. Once established, the transport is used to migrate EGFX graphics traffic off TCP: request_reliable_udp is called opportunistically the first time EGFX has data to send (its dynamic channel id is only known once the client opens it, well after multitransport bootstrapping), and outgoing EGFX SvcMessages route onto the tunnel via encode_unframed_pdu() once the client has acknowledged the Soft-Sync request. A new client_loop select arm feeds incoming tunnel payloads into DrdynvcServer::process_tunnel(), whose responses go back over TCP. A closed transport degrades the arm to pending forever rather than ending the session, matching the non-fatal posture throughout. Adds a regression test verifying that configuring UDP transport on the server does not disturb a client that never advertises support for it (the common case for any client that predates this feature).
Fixes three high-severity issues: Soft-Sync now requires both a negotiated SOFT_SYNC_TCP_TO_UDP flag and a successful Initiate Multitransport Response before migrating any channel, the shared UDP transport handle exposes a lock-free sender independent of its receive-side mutex, and the finalize handler no longer blocks the RDP handshake on the UDP accept, spawning it instead and picking it up opportunistically from client_loop's own select loop once it resolves. Fixes a medium-severity bug in ironrdp-dvc's Soft-Sync response handling: a declined channel stayed routed for outgoing data because the outgoing tunnel map was never filtered by the response, only the incoming one. Addresses four low-severity findings: corrects a false single-connection premise in the UDP accept doc comment, documents the AddrInUse tradeoff under session preemption, combines a duplicated drdynvc guard into one failure path, and confirms two findings already resolved by rebasing onto PR Devolutions#1953's own review-response commit.
Fixes a high-severity bug: after the sideband UDP tunnel closes, the shared transport handle now gets cleared so dispatch_egfx_messages actually falls back to TCP instead of silently dropping every subsequent EGFX batch onto a dead connection. Documents an accepted timing limitation: a late Initiate Multitransport Response arriving after finalization completes cannot retroactively enable Soft-Sync migration, since nothing on the message channel recognizes it post-handoff. This degrades to TCP-only for the session rather than causing any correctness issue. Inherits the S_OK/SOFTSYNC test fix from PR Devolutions#1953 by rebasing onto its review-response commit, reconciling the resulting connection.rs conflict between that PR's bool-returning rename and this branch's own earlier &mut self change for response tracking. Addresses three low-severity findings: removes an unused accessor, substitutes an equivalent enum match with the existing tls_acceptor() helper, and reuses get_svc_processor() instead of inlining its body.
Greg Lamberson (glamberson)
force-pushed
the
feat/server-multitransport-wiring
branch
from
September 12, 2026 05:03
0be1959 to
5a788d2
Compare
Greg Lamberson (glamberson)
deployed
to
llm-providers
September 12, 2026 05:03 — with
GitHub Actions
Active
GitHub Actions
Active
github-actions
Bot
added
the
maintainer-required
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on #1953
This PR is stacked on #1953 (
feat/acceptor-multitransport-finalize), itself stacked on #1951. GitHub does not support cross-fork stacked PRs, so this diff is filed againstmasterand is cumulative with both; the incremental diff is atlamco-admin/IronRDP/compare/feat/acceptor-multitransport-finalize...feat/server-multitransport-wiring.Summary
RdpServerBuilder::with_udp_transport(udp_bind_addr): opt-in,Noneby default, no behavior change unless called.TlsorHybrid, matching the reference client's Enhanced-Security-only gate), the acceptor offers UDP multitransport, andaccept_finalizeusesaccept_finalize_with_multitransportwith a callback that binds a fresh UDP socket per connection, reuses the connection's own TLS certificate (TlsAcceptor::config()) for the sideband transport, and callsaccept_udp().request_reliable_udpis called opportunistically the first time EGFX has data to send (its dynamic channel id is only known once the client opens it), and outgoing EGFX messages route onto the tunnel viaencode_unframed_pdu()once the client has acknowledged the Soft-Sync request. A newclient_loopselect arm feeds incoming tunnel payloads intoDrdynvcServer::process_tunnel().Validation
cargo xtask check fmt/lints/tests/typos/locksall pass, including--features egfxand--no-default-features --features helperspecifically (the new code has real feature-gated branches).Added a regression test to
ironrdp-testsuite-extraconfirming that configuring UDP transport on the server does not disturb a client that never advertises support for it (the common case for any client predating this feature).Notes
Two things caught and fixed while implementing this, not left in the diff:
tunnel_for_outgoing_channelbookkeeping, which is set synchronously on request, before the client acknowledges. Sending frame data over the tunnel before the client processed the Soft-Sync request would land on a receive path it had not set up yet. Fixed by additionally gating onsoft_sync_response_received().SvcMessage(the SoftSyncRequest PDU) thatrequest_reliable_udp()returns, meaning the server would decide to migrate but never actually tell the client. Fixed to encode and send it over TCP.Full tunnel-establishment coverage (a real client completing the UDP handshake end to end) isn't in this PR: the shared e2e harness's client side doesn't yet drive
connect_finalize_with_multitransport. Wiring that up is a natural, separate follow-up rather than scope creep here.