Skip to content

fix(dvc): switch a Soft-Sync tunnel that also lists declined channels - #2007

Merged
Marc-André Moreau (mamoreau-devolutions) merged 1 commit into
Devolutions:masterfrom
AKolenda:fix/dvc-soft-sync-unopened-channels
Sep 28, 2026
Merged

Marc-André Moreau (mamoreau-devolutions) merged 1 commit into
Devolutions:masterfrom
AKolenda:fix/dvc-soft-sync-unopened-channels

Conversation

@AKolenda

@AKolenda AKolenda commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

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.

A Soft-Sync request lists every dynamic channel the server intends to move,
including channels the client answered with NO_LISTENER. Windows lists
CoreInput, MouseCursor, Video and Geometry next to the graphics pipeline.
Any unopened channel in a list made the client drop that whole list, so
the tunnel was never switched, and the channels the client had opened
stayed on TCP while the server was already sending them on the tunnel.

Skip unopened channels one by one and switch the tunnel for the rest.
Copilot AI balanced review requested due to automatic review settings September 26, 2026 06:24

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

Copy link
Copy Markdown
Contributor

This pull request may overlap with #2008.

Both touch the DRDYNVC client's Soft-Sync handling in crates/ironrdp-dvc/src/client.rs for the same scenario: Windows moving dynamic channels onto the tunnel while referencing channels the client declined with NO_LISTENER. This PR skips unknown channel IDs in the Soft-Sync list; PR 2008 handles Create/Close and NO_LISTENER response routing around the same tunnel-binding logic, so shared scope merits human review.

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).

@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.

🟢 PR #2007 rewrites process_soft_sync_request so channel IDs the client never opened (answered NO_LISTENER) are logged and skipped instead of voiding the whole tunnel list, and every available listed tunnel is acknowledged. Independent review confirms correctness: the unavailable-tunnel guard (MS-RDPEDYC 3.2.5.3.1) is preserved, tunnel_channels only maps opened channels, per-channel routing checks in process_tunnel and the TCP data guard stay consistent, and the request decoder already rejects duplicate channel IDs and tunnel types. Acknowledging tunnels whose listed channels were all declined matches the in-repo server validation and avoids stranding opened channels on TCP while the server sends on the tunnel. The new test reproduces the Windows 11 list shape. Both valid specialists (protocol, skeptical) reported zero findings; the code-compressor specialist failed and produced none. No findings are published.

Reduced coverage: optional reviewer code-compressor was unavailable.

@github-actions github-actions Bot added ai-reviewed/1 One automated review completed needs-review A human reviewer is the current next actor labels Sep 26, 2026
@AKolenda

Copy link
Copy Markdown
Contributor Author

On the overlap notice: #2007 and #2008 are complementary and merge cleanly in either order. #2007 decides which channels the Soft-Sync response binds when the request also lists channels the client declined. #2008 handles the Create, Close and graphics traffic that arrives on the tunnel after the switch. Against Windows 11 the graphics pipeline needs both to work over the tunnel.

@mamoreau-devolutions
Marc-André Moreau (mamoreau-devolutions) merged commit 3b3a17b into Devolutions:master Sep 28, 2026
51 of 55 checks passed

This branch was successfully deployed

1 active deployment
llm-providers — 2fd0674d Deployed Sep 26, 2026 by AKolenda via Classify pull request #679
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 kind/protocol Affects RDP or related protocol behavior needs-review A human reviewer is the current next actor risk/medium Behavioral change that does not substantially alter a core public API scope/core Touches the core architectural tier size/S Size: up to 199 counted lines and 5 files; exceeds XS in either measure triage/overlap Possible overlap with another pull request; advisory only

Development

Successfully merging this pull request may close these issues.

3 participants