Skip to content

fix(client): send shutdown before resize-fallback reconnect - #1804

Open
Mariusz Białończyk (manio) wants to merge 2 commits into
Devolutions:masterfrom
manio:connection-fixes
Open

fix(client): send shutdown before resize-fallback reconnect#1804
Mariusz Białończyk (manio) wants to merge 2 commits into
Devolutions:masterfrom
manio:connection-fixes

Conversation

@manio

Copy link
Copy Markdown
Contributor

Hi
This is my series of commits that fixes the connection issues to the server. Unfortunately, one of my target hosts is not exactly a speed demon, and to make matters worse, my corporate setup adds full antivirus protection and other stuff on top of it. As a result, only maybe 2–3 out of 10 connection attempts would succeed.

You had to hit the timing just right because IronRDP’s initial sequence was quite aggressive (interestingly, FreeRDP didn’t have this issue).

These two commits improve the situation dramatically - now virtually all the connections I've tried work reliably.

DISPLAY_CONTROL_READY_TIMEOUT was 3s, too tight right after login:
the server is still negotiating DVC channels (rdpdr, rdpei,
geometry, displaycontrol) at that point and can miss the
Deactivate-Reactivate window, causing spurious ReactivationTimedOut
fallbacks. Raise it to 8s.

The reconnect loop also fired a brand new NLA/CredSSP handshake
immediately after a resize fallback, with zero delay. Landing right
after a prior handshake in this same loop, some RDP hosts reject the
follow-up logon with STATUS_LOGON_FAILURE even though the
credentials are correct. Add a 2s cooldown before retrying, mirroring
the delay already used by confirm_auto_reconnect for transport-
failure retries.
@github-actions github-actions Bot added maintainer-required Maintainer review or intervention is required risk/unknown Risk could not be determined automatically; needs maintainer-level scrutiny size/S Size: up to 199 counted lines and 5 files; exceeds XS in either measure labels Aug 26, 2026
@manio Mariusz Białończyk (manio) changed the title Connection fixes fix(client): send shutdown before resize-fallback reconnect Aug 28, 2026
A resize fallback abandons the current session by dropping the TCP
connection, with no RDP-level signal that the session ended. If a
fresh NLA/CredSSP logon for the same account arrives moments later,
the server can still consider the old session live, stall for
several seconds resolving the conflict, and then reject the new
logon with STATUS_LOGON_FAILURE.

Send a ShareDataPdu::ShutdownRequest (graceful_shutdown) before each
of the four reconnect-with-new-size return paths. This is
fire-and-forget with a 500ms write timeout; we don't wait for the
server's ShutdownDenied PDU since the client is reconnecting either
way.
@github-actions github-actions Bot added breaking-change Includes a breaking change, and requires special scrutiny at the boundaries risk/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny and removed risk/unknown Risk could not be determined automatically; needs maintainer-level scrutiny labels Aug 28, 2026
@manio

Copy link
Copy Markdown
Contributor Author

I also ran some additional tests in a completely different environment, and the results were actually as expected (although somewhat counterintuitive, which is a good thing). Increasing the timeout to 8 seconds makes the session appear around 5-6 seconds faster.
I did around 30 test connections using both the original 3-second value and the new 8-second value, and the results were consistent. So, from my perspective, the change looks absolutely valid.

p.s. Is this PR good to merge?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change Includes a breaking change, and requires special scrutiny at the boundaries maintainer-required Maintainer review or intervention is required risk/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny size/S Size: up to 199 counted lines and 5 files; exceeds XS in either measure

Development

Successfully merging this pull request may close these issues.

1 participant