feat(server): let an authenticated connection preempt an existing session by antonmos · Pull Request #1476 · Devolutions/IronRDP

Anton Mostovoy (antonmos) · 2026-07-26T17:34:30Z

Summary

RdpServer::run serves one connection at a time: while run_connection is awaited for a client, a second inbound connection sits unserved in the TCP listen backlog until the first session ends — a silent hang from that client's point of view. There is currently no way for a server to say what should happen instead.

This adds an opt-in preempt_existing_session (default false, so behaviour is unchanged unless a caller opts in): when set, a newcomer that has completed authentication takes the session over, and the incumbent is told why before it is dropped. Intended for a server backing one specific session (mirroring a single desktop) rather than many short-lived connections.

Security model

Evicting a live session is disruptive, so the bar is completed authentication, not "looks like RDP". A candidate runs the full negotiation — and CredSSP under Hybrid — before the live session is touched at all; a candidate that fails at any step leaves it untouched. The guarantee genuinely varies by security mode, so it is documented rather than implied:

Mode	Bar to preempt	Guarantee
`Hybrid`	CredSSP/NLA succeeds	an unauthenticated peer can never evict
`Tls`	TLS handshake completes	partial — a `CredentialValidator` still runs later, during finalization
`None`	X.224/MCS negotiation completes	none — the mode performs no authentication

Candidates are additionally gated through ConnectionHandler::on_accept before they may negotiate (and exactly once per connection, since that hook is stateful for rate limiters), so an allowlist or limiter can stop a takeover rather than only learning of it afterwards.

An earlier revision of this PR gated on a two-byte TPKT peek. That was too weak — see the review discussion: a well-formed connection evicting a session that was still mid-CredSSP is the failure that motivated the rewrite, and CR-validation alone would not have caught it.

Shape

Rather than duplicating the pre-finalization half of run_connection for the candidate path, it is extracted and shared:

negotiate_and_authenticate — a free function (&RdpServerSecurity + &mut Acceptor, no &mut self, which is what lets it run concurrently with the live connection's borrow). Both run_connection_with and the candidate path go through it, so there is one negotiation implementation.
attach_channels_impl — the channel-attach body, returning the GFX handle so only a winning candidate installs it.
Factories Box → Rc so a candidate holds cheap clones. Builder API unchanged (still takes Box).
finalize_after_upgrade → finalize_and_shutdown.

Eviction notice

Eviction sends the incumbent ERRINFO_DISCONNECTED_BY_OTHERCONNECTION (MS-RDPBCGR 2.2.5.1.1 — what real Windows RDS sends on takeover) before disconnecting, with a bounded grace so it reaches the wire.

This is load-bearing now that the Server Auto-Reconnect Cookie has landed (#1405): without a reason, an evicted client auto-reconnects ~1 s later, re-authenticates, preempts the client that replaced it, and the two ping-pong indefinitely (observed downstream at ~1–2 s per cycle). As a net under it — whether a client honours the notice is client-dependent — a just-evicted peer may not immediately re-preempt, and each refused attempt re-arms the window, so a reconnect storm can never win while a deliberate reconnect still can.

Testing

All in-tree, each verified to fail without its fix:

traffic that merely looks like RDP, and a bare connect-and-close, never become eligible candidates;
the eviction notice round-trips as a properly framed Share Data PDU carrying the takeover code;
the anti-storm window re-arms under a reconnect storm but lets a quiet peer back in;
a run-loop test proves a handler-rejected candidate never evicts the live session.

cargo test, cargo clippy --all-targets and cargo fmt --check are clean for ironrdp-server with and without egfx, and cargo build --workspace passes. The full-auth positive path is exercised downstream against real clients, since it needs a live NTLM/CredSSP client.

Pull request overview

Adds opt-in session preemption so a new RDP client can replace an active connection.

Changes:

Adds configurable preemption through server options and builder API.
Probes candidate connections for a TPKT header before preempting.
Adds loopback tests for probe classification and timeout behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
`crates/ironrdp-server/src/server.rs`	Implements probing, preemption control flow, options, and tests.
`crates/ironrdp-server/src/builder.rs`	Exposes and initializes the preemption option.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…ssion (#174) * fix(server): a second client hangs instead of taking over the live session RdpServer::run (vendored ironrdp-server) serves one connection at a time: while a session is live, a second client's TCP connect just sits unserved in the listen backlog until the first session ends -- a silent hang from that client's side. macrdp is single-console-session by design (it mirrors one desktop), so there's no reason a fresh connect shouldn't just take over. Reshape the accept loop so an in-flight connection races listener.accept(): a candidate that clears on_accept (the auth guard's per-source-IP rate-limit/lockout) AND proves it's actually starting an RDP handshake (peeked TPKT header, within a short timeout, so a bare connect or port scan can't kill a live session) causes the current session to be dropped -- cancelled, the same teardown a client-side disconnect takes -- and the candidate served in its place on the next loop iteration. on_accept is gated on the candidate as soon as it's accepted, BEFORE it's allowed to start the TPKT probe -- and so before it can ever preempt anything -- rather than only on the next outer-loop iteration after the live session was already cancelled, which would let a candidate the guard would reject still evict the active session before being rejected. And it's gated exactly ONCE per physical connection: AuthGuardHandler::on_accept is stateful (records the accept toward its rate-limit window, writes an audit line), so a winning candidate must not run it again when served from the pending slot after already clearing it once during the race. Covered by three conn_test cases, each verified to fail without its corresponding fix and pass with it: - second_client_preempts_the_live_session (drives the real RdpServer::run accept loop over real TCP -- every other conn_test uses an in-memory duplex, since preemption lives in the accept loop itself) - preemption_does_not_evict_a_session_the_handler_would_reject - a_preempting_candidate_clears_on_accept_exactly_once Filed upstream as Devolutions/IronRDP#1476 (an opt-in preempt_existing_session option, since a general-purpose server would want to keep queuing behind); this is the always-on macrdp-specific shape until that lands and the vendor pin can adopt it -- see divergence (22) in vendor/ironrdp-server/CLAUDE.md. * fix(server): stop the preemption race from disabling the auth/fingerprint hooks The previous commit's connection_handler.take()/restore trick (to let a preempting candidate's on_accept run while the live connection's run_connection future also holds &mut self) took connection_handler OUT of self for the entire race window -- not just an instant. But run_connection itself reads self.connection_handler mid-connection, for on_authenticated (CredSSP outcome) and on_client_fingerprint (client name/version/platform), both called from deep inside accept_finalize while the race loop still has it checked out. Since macrdp's preemption is unconditional (every connection goes through the race loop, not just ones that actually get preempted), this silently broke BOTH hooks for every single connection -- caught by the "audit log (macos integration)" CI job: zero event="auth" records for either the correct- or wrong-password sdl-freerdp attempt. Fix: store connection_handler as Rc<RefCell<Box<dyn ConnectionHandler>>> instead of a bare Option<Box<..>>. A cheap Rc clone taken before the race gives the candidate's on_accept check a handle to the SAME handler without ever removing it from self, so run_connection's own on_authenticated/on_client_fingerprint calls keep working throughout. Removes the take()/restore dance entirely -- simpler than the code it replaces. Public API unchanged (with_connection_handler still takes an owned Box; RdpServer::new wraps it internally). Not locally reproducible in this sandbox (no Screen Recording TCC grant for the full macrdp binary, which the audit-log script drives directly), but the two broken call sites were confirmed by direct inspection of the connection_handler read sites inside run_connection's call graph -- both fully explain the observed symptom. Verified via cargo build/test/clippy/fmt; the CI job itself is the authoritative check for this fix. * fix(server): gate preemption on full authentication, not a TPKT peek CI caught a real regression in the previous commit's mechanism: the accept loop raced listener.accept() from the moment a connection started being served, not just once it was fully active. In scripts/test-audit-log.sh's two sequential connections (correct password, then wrong password), the second connection could win the TPKT-peek race -- 2 bytes, near-instant -- before the first connection's real TLS+CredSSP handshake completed, cancelling a session that was about to authenticate successfully. The audit log's expected event="auth" outcome="success" for connection 1 never appeared. This isn't just a benign-overlap edge case: it means ANY connection attempt, including one that never authenticates at all, could evict the live session purely by winning a race against a 2-byte peek -- backwards from what preemption should guarantee. Confirmed requirement: an unauthenticated connection must never be able to disconnect the live session. Replace the TPKT probe with real negotiation: a candidate must complete TLS and (where the security mode is Hybrid) CredSSP authentication before it's allowed to preempt anything. Since RdpServer serves one connection at a time by design (single static_channels, single gfx_handle, factories that build real per-connection backends against shared hardware -- capture, camera, USB, RDPDR/NFS) and run_connection holds &mut self for its whole lifetime, a candidate's negotiation has to run without touching self mutably at all to run concurrently with the live connection. Split into a Phase 1 (negotiate + authenticate, concurrency-safe via a cloned NegotiationContext) and Phase 2 (exclusive, resource-driving, only for the winner): - Factory storage changed Box<dyn X> -> Rc<dyn X> (cliprdr, sound, rdpdr, usb, camera, gfx) so a candidate's negotiation can hold cheap clones instead of needing self. Public builder API unchanged. - attach_channels_impl: the channel-attaching logic factored out to take explicit references instead of reading self, shared by both the normal path and negotiate_candidate. - negotiate_candidate: duplicates the pre-accept_finalize portion of run_connection (negotiate -> attach channels -> TLS -> CredSSP) against a NegotiationContext instead of &mut self. Returns Some only on full success; any failure leaves the live session untouched, and fires on_authenticated on the candidate's outcome too, so a rejected preemption attempt still shows up in the audit log. - Multitransport (the UDP offer + lossy-audio DVC) and RTT sampling are skipped for a candidate -- that state is process-wide and shared with the live connection's own bookkeeping, not safe to touch concurrently. A preemption winner always negotiates plain TCP. - serve_negotiated: Phase 2 for a winner -- installs the deferred GFX handle, resets auto_reconnect_sent, hands off to the same accept_finalize the normal path uses. - run()'s race loop: pending now carries a fully negotiated candidate, never a raw stream. conn switched from stack-pinned to Box::pin, since it's now either a fresh connection or a previously-preempting candidate that's itself now live and must stay preemptible by a third candidate -- two future types unified behind one dyn Future. Existing conn_test coverage (second_client_preempts_the_live_session, preemption_does_not_evict_a_session_the_handler_would_reject) still passes unchanged, proving the positive path and the on_accept gate. Not covered locally: a candidate that reaches CredSSP but fails it -- ironrdp-server isn't a workspace member here, so #[cfg(test)] code inside the vendor crate can't run via any cargo test invocation from macrdp. scripts/test-audit-log.sh (real sdl-freerdp, correct AND wrong password) is exactly this scenario and is the authoritative check, run in CI. See divergence (23) in vendor/ironrdp-server/CLAUDE.md for the full writeup. * fix(server): tell an evicted client why, or two clients ping-pong forever Live testing found preemption working exactly as designed and still unusable: two real LAN clients traded the session back and forth every ~1-2 seconds, indefinitely. The loop is self-inflicted. macrdp provisions the Server Auto-Reconnect Cookie by default (so a blank-recovery drop heals seamlessly), which means a client dropped for ANY reason silently auto-reconnects about a second later. A preemption drop was indistinguishable from a blank-recovery drop, so the evicted client came straight back, authenticated -- legitimately; it's a real client with real credentials, so the new auth gate is no defense here -- and preempted the client that had just replaced it. Repeat forever. Every component behaved correctly on its own; only the composition was broken. Two layers: 1. ServerEvent::EvictedByOtherConnection (the real fix). A new event distinct from Quit: dispatch_server_events sends a Server Set Error Info PDU carrying ERRINFO_DISCONNECTED_BY_OTHERCONNECTION (0x05, MS-RDPBCGR 2.2.5.1.1 -- the code real Windows RDS uses for a session takeover) before disconnecting. A client given an administrative disconnect reason shows it and does not auto-reconnect. run() sends this and keeps polling the incumbent for EVICTION_GRACE (750ms) so the PDU reaches the wire, degrading to the previous hard cancellation on timeout so a half-dead peer can't stall the takeover. 2. recently_evicted + REPREEMPT_COOLDOWN (the net). Whether a client honors the error info is client-dependent and unverified in the field, and an infinite flap is bad enough to be worth a structural bound. A just-evicted peer may not immediately preempt back: keyed on source IP (the source port changes every reconnect), 5s, and each refused attempt re-arms the window -- so an auto-reconnect storm can never win no matter how long it runs, while a human who closes and reconnects still can. Known limitation: IP-keyed, so two clients behind one NAT briefly block each other's takeover after an eviction; accepted, since it's a net under the real fix. second_client_preempts_the_live_session now decodes the PDU and asserts the evicted session actually receives the eviction reason, rather than just checking the socket closed. New a_just_evicted_peer_cannot_immediately_preempt_back reproduces the live ping-pong. Both verified to fail without their fix and pass with it. See divergence (23) in vendor/ironrdp-server/CLAUDE.md.

clintcan · 2026-07-29T09:38:36Z

Flagging a security concern with the preemption gate before this lands — and pointing at a fix that already exists downstream.

The issue: probe_preempting_client gates on a TPKT-header MSG_PEEK, which — as the PR body says — "is not authentication." The consequence is that any unauthenticated connection can evict a live, authenticating session by winning the two-byte race. A peer that sends a plausible TPKT prefix (a port scanner, a stray half-open probe, or a deliberately malicious connect) cancels the in-flight session before it ever has to prove who it is. That's backwards from what preemption should guarantee: dropping a live desktop session is disruptive, so the bar to do it should be higher than "authenticated," not lower.

It's not hypothetical. The downstream macrdp implementation this derives from started with exactly this peek mechanism and hit it as a real CI failure. scripts/test-audit-log.sh drives two sequential connections (correct password, then wrong); after the peek-based version shipped, the first connection's event="auth" outcome="success" stopped appearing — because the second sdl-freerdp process won the TPKT-peek race (2 bytes, near-instant) and cancelled the first connection while its TLS+CredSSP handshake was still completing. So the peek let an unrelated connection kill a session that was about to authenticate successfully.

The suggested on_accept mitigation doesn't close it. ConnectionHandler::on_accept bounds attempt frequency, not authentication — a single well-timed connect from a fresh source IP still passes the rate limiter and wins the race.

The fix (already implemented downstream): a candidate must complete real negotiation — TLS, and CredSSP where the security mode is Hybrid — before it may preempt anything; a candidate that fails at any step returns without touching the live session. The one structural wrinkle is that the candidate's negotiation has to run without &mut self so it can proceed concurrently with the live connection's borrow — a cheap cloned negotiation-context snapshot plus factories stored behind Rc does it, and the winner finalizes afterward. macrdp also adds an anti-storm guard so a just-evicted peer can't immediately bounce back and re-take the slot.

I opened #1483 laying out the invariant ("an unauthenticated connection must never evict a live session") and the reject/queue/preempt policy question before I realized this PR was already open — apologies for the parallel track. Happy to help land the full-auth gate here, or to fold #1483's discussion into this PR, whichever you'd prefer.

Benoît Cortier (CBenoit) · 2026-08-03T08:24:12Z

Blocking finding: the preemption probe accepts only the 03 00 TPKT prefix, allowing malformed or non-RDP traffic to evict an active session before failing later in the handshake; validate the complete TPKT framing and X.224 Connection Request before preempting.

clintcan · 2026-08-03T08:52:08Z

Agree the 03 00 peek is too weak. One thing on the proposed fix, though: validating the full TPKT + X.224 Connection Request closes the malformed-traffic hole, but a well-formed CR is still unauthenticated — it's the pre-TLS/pre-CredSSP first packet, so any peer (a port scanner, a deliberately crafted connect) can present a valid CR and still evict a live session. If the invariant is "untrusted traffic must not evict an authenticated session," CR-validation is necessary but not sufficient; the bar has to be completed authentication.

That's the shape macrdp settled on (#1483 lays out the invariant; the downstream impl gates preemption behind TLS and CredSSP before touching the live session). It surfaced concretely as a CI failure: a well-formed second connection evicted a session that was mid-CredSSP. Happy to help wire the full-auth gate here if that's useful.

Anton Mostovoy (antonmos) · 2026-08-03T17:55:08Z

Rewritten on current master (also resolves the conflict). Benoît Cortier (@CBenoit)'s blocking finding and clintcan's follow-up are both addressed, but by a stronger gate than validating the Connection Request — because CR-validation alone would not have caught the failure this actually produces in the field.

Why CR-validation isn't sufficient. It does close the malformed-traffic hole. But a well-formed CR is the pre-TLS/pre-CredSSP first packet, so any peer can present one — and the concrete failure downstream was not malformed traffic at all: a perfectly valid second sdl-freerdp connection evicted a session that was still mid-CredSSP, and the first connection's auth outcome="success" audit event vanished. Full CR parsing would have let that through unchanged.

What it does now. A candidate must complete real negotiation — and CredSSP under Hybrid — before the live session is touched at all; any failure leaves it untouched. Since the guarantee genuinely varies by security mode, the option documents it rather than implying it's uniform:

Mode	Bar to preempt	Guarantee
`Hybrid`	CredSSP/NLA succeeds	an unauthenticated peer can never evict
`Tls`	TLS handshake completes	partial — a `CredentialValidator` still runs later, during finalization
`None`	X.224/MCS negotiation completes	none — the mode performs no authentication

Open question for you: the Tls + CredentialValidator row is a residual I deliberately did not paper over. Tightening it means pulling validation forward out of accept_finalize, which is &mut self territory — happy to do that if you'd prefer, or to restrict preemption to Hybrid outright.

No duplicated negotiation. clintcan flagged that the downstream implementation duplicates the pre-accept_finalize half of run_connection and that this "would be designed away if this lived upstream" — so it is: negotiate_and_authenticate is a free function (&RdpServerSecurity + &mut Acceptor, no &mut self, which is what lets it run concurrently with the live connection's borrow) and both run_connection_with and the candidate path go through it. attach_channels is likewise factored into a free attach_channels_impl returning the GFX handle so only a winner installs it, and the factories move Box → Rc so a candidate can hold cheap clones (builder API unchanged). finalize_after_upgrade shrinks to finalize_and_shutdown.

One finding worth flagging, not previously in either thread. Now that the Server Auto-Reconnect Cookie has landed (#1405), preemption without an eviction reason produces an infinite ping-pong: each evicted client auto-reconnects ~1 s later, re-authenticates, and preempts the client that replaced it — forever. Observed live downstream at ~1–2 s per cycle with two clients. So eviction now sends ERRINFO_DISCONNECTED_BY_OTHERCONNECTION (2.2.5.1.1 — what real RDS sends on takeover) before disconnecting, with a bounded grace so it reaches the wire.

This interacts with the cookie in a way that may deserve its own look independently of this PR: server.rs's validator call is gated if !is_auto_reconnect && …, so a reconnecting client bypasses CredentialValidator — meaning an evicted client can bounce back without re-validating. As a net under the notice (whether a client honours it is client-dependent), a just-evicted peer may not immediately re-preempt, and each refused attempt re-arms the window, so a reconnect storm can never win while a deliberate reconnect still can.

Tests (all in-tree, and each verified to fail without its fix): traffic that merely looks like RDP and a bare connect-and-close never become eligible candidates; the eviction notice round-trips as a properly framed Share Data PDU with the takeover code; the anti-storm window re-arms under a storm but lets a quiet peer back in; and a run-loop test proves a handler-rejected candidate never evicts the live session. The full-auth positive path is exercised downstream against real clients rather than here, since that needs a live NTLM/CredSSP client.

Re #1483 — happy to fold this into whatever policy shape you land on there (enum vs. exposed hooks); this PR deliberately stays a single opt-in flag so it doesn't pre-empt that decision.

🤖 Addressed by Claude Code

Anton Mostovoy (antonmos) · 2026-08-03T18:05:09Z

Heads-up on the red Checks [windows]: the failure is ironrdp-agent's daemon_reports_no_active_session (crates/ironrdp-agent/tests/ipc.rs:54, "daemon did not become ready"), which I don't believe this PR can reach:

ironrdp-agent does not depend on ironrdp-server, directly or transitively — it pulls ironrdp-client/-core/-pdu/etc., and the only ironrdp_server string in the crate is help text in cli.rs.
This PR touches only crates/ironrdp-server/src/{server,builder}.rs.
The test spawns the agent binary and polls agent status against a 10 s deadline; the job reports 29.61 s for it, which reads like a loaded runner rather than a logic failure.

Every other check passed, including Checks [linux], Checks [macos] and the full feature matrix.

I'd normally just re-run to confirm rather than assert it's flaky, but I don't have permission on this repo (Must have admin rights to Repository) — could someone kick that job? I'd rather have it re-run than push a no-op commit to this PR's history. For what it's worth it's the only Checks [windows] failure in the last 33 completed runs, and I found no prior report of this test failing, so I can't call it a known flake — just one I can't tie to this change.

🤖 Addressed by Claude Code

clintcan · 2026-08-03T22:59:33Z

This is the right resolution — thanks for taking it to full-auth, Anton Mostovoy (@antonmos). Two data points from the macrdp side that corroborate the new findings:

The eviction notice matches what we shipped: with the ARC cookie live, preemption without ERRINFO_DISCONNECTED_BY_OTHERCONNECTION + a bounded grace does produce the ~1–2 s reconnect ping-pong, and the notice + an anti-storm window is what stops it. So that's independent confirmation the Share-Data-PDU takeover code is the correct one, not just plausible.
On the validator-bypass-on-reconnect (if !is_auto_reconnect skipping CredentialValidator): agree it deserves its own issue, and worth noting the exposure is specific to that seam. A static-credential server re-runs CredSSP/NLA on the RDP-layer auto-reconnect (the client re-auths from cached creds before it completes), so that path stays authenticated — it's only the CredentialValidator-installed path that's skipped. So the fix likely belongs at the is_auto_reconnect gate, independent of preemption.

No opinion to add on the Tls+validator residual or the #1483 policy shape — those are your calls, Benoît Cortier (@CBenoit).

RdpServer::run_connection_with inlines the whole negotiate-then-finalize sequence as `&mut self` methods, which makes it impossible for any future caller to drive that same negotiation without holding a mutable borrow of the whole server for the duration. Extract the reusable pieces as free functions that take only what they need instead of `self`: - `negotiate_and_authenticate`: everything from `accept_begin` through the optional Hybrid CredSSP exchange, i.e. everything up to (but not including) `accept_finalize`. Returns a `NegotiatedTransport<S>` enum that captures which of the three finalize behaviours applies (no upgrade / TLS upgrade / already-offloaded TLS), preserving the exact existing behaviour for each. - `attach_channels_impl`: the channel-attaching half of connection setup, factored out of `RdpServer::attach_channels`, taking borrowed factories instead of reading `self`. Returns the GFX handle instead of writing it to a field, so the caller decides when to install it. `RdpServer::attach_channels` and `run_connection_with` become thin wrappers around these. `finalize_after_upgrade` (which used to also perform the security-upgrade marking and CredSSP exchange, now both moved into `negotiate_and_authenticate`) is renamed to `finalize_and_shutdown` and shrinks to just `accept_finalize` + the stream shutdown, reflecting its narrower remaining job. The `sound_factory` / `cliprdr_factory` / `gfx_factory` fields move from `Box<dyn _>` to `Rc<dyn _>`. The public builder API is unchanged (still takes `Box`, wrapped via `Rc::from` in `RdpServer::new` after the existing `set_sender` wiring, which needs `&mut` on the owned `Box`); internally this lets a future connection-setup path build its channels from a cheaply cloned reference to these instead of reaching through `self`. No behavior change: this is a pure extraction, verified with `cargo test -p ironrdp-server` (default and `--features egfx`) and a full `cargo build --workspace`.

Anton Mostovoy (antonmos) · 2026-08-08T16:00:34Z

Per the size-bot's request, I've split the shared-negotiation refactor out into its own PR: #1588.

#1588 contains only the behavior-preserving extraction (negotiate_and_authenticate / attach_channels_impl free functions, Box→Rc factories, finalize_after_upgrade→finalize_and_shutdown) — no preemption logic, no new tests needed since nothing changes. It's the piece that lets this PR's candidate-negotiation path run without holding &mut self for the whole negotiation.

Once #1588 merges, I'll rebase this PR on top of it and trim the diff down to just the feature-specific code: the preempt_existing_session option, the run() preemption race, negotiate_candidate/serve_negotiated, the ERRINFO_DISCONNECTED_BY_OTHERCONNECTION eviction notice, the anti-storm guard, and their tests. That should bring this PR comfortably under the size threshold.

🤖 Addressed by Claude Code

Two purely additive conflicts against master's Devolutions#1684/Devolutions#1685 (configurable RemoteFX quantization table, unrelated to preemption): RdpServerOptions and its builder-state counterpart each gained a new remotefx_quant: Quant field alongside this branch's own new preempt_existing_session: bool field -- both fields kept. Also added the new field to the one hand-built RdpServerOptions literal this branch has (the preempt_tests test-context helper), which the auto-merge couldn't do for me since master has no equivalent code to diff against there. 23/23 tests pass in both default and --features egfx, clippy --all-targets clean in both, fmt clean, full cargo build --workspace green.

…, keyboard metadata, entropy coder) Three upstream commits since the last sync, all in crates/ironrdp-server: Devolutions#1691 (feat!, keyboard metadata via ConnectionHandler::on_connection_info) merged with no conflict -- purely additive (a new struct, a new default trait method, one call site in client_accepted, which this branch never touches). Devolutions#1721 (fix, release static channels when a connection ends) restructured run_connection_with itself, which this branch also restructured (into PendingConnection-based negotiation) -- git's merge conflicted on run()'s whole preemption-race loop and, separately, split my mod preempt_tests and Devolutions#1721's own new mod tests into two overlapping conflict hunks (same file-tail location, different module names). Resolved by taking this branch's side for the run() loop (a superset of upstream's simpler non-preemption tail -- same on_disconnected call, just wrapped in the preemption if/else) and keeping BOTH test modules side by side. One splice bug caught by the build: mod preempt_tests was left without its own closing brace; fixed. The real interaction worth documenting: Devolutions#1721's fix assumes run_connection_with is the only connection entry point, but this branch adds a second one, serve_negotiated (the preemption-winner path), which never calls run_connection_with. Removing run()'s own static_channels reset -- which upstream's fix intends, since run_connection_with now resets it universally -- would have silently reintroduced Devolutions#1721's exact leak, just scoped to every preemption takeover instead of every run_connection embedder. Kept run()'s reset, with a comment explaining why it is NOT redundant despite resetting the same field twice on the run_connection path. Devolutions#1685/Devolutions#1684 (RemoteFX quant, already merged in c831bcc) plus this sync's new Devolutions#1686 (entropy coder weighting) add a second new RdpServerOptions field, remotefx_entropy_coder: Option<EntropyBits> -- added to the one hand-built RdpServerOptions literal in preempt_tests, same as remotefx_quant before it. Verification, in order, catching problems at each step rather than trusting a clean `cargo build`: plain build was clean but does not compile #[cfg(test)] code, so `cargo test` is what actually caught the missing remotefx_entropy_coder field. 24/24 tests pass in both default and --features egfx (up from 23 -- upstream's own run_connection_releases_the_static_channels regression test now runs against this branch's PendingConnection-based negotiation and passes, which is strong evidence the merge is semantically correct, not just textually conflict-free). clippy --all-targets clean in both configs, fmt clean, full cargo build --workspace green.

…e public API) Six upstream commits since the last sync, in crates/ironrdp-server: Devolutions#1242 (typed ServerError, breaking), Devolutions#1417 (rdpeusb server integration), Devolutions#1773 (RDPEI wiring), Devolutions#1737 (RTT baseline), Devolutions#1769 (mouse position, no server.rs footprint), Devolutions#1754 (test relocation, no server.rs footprint). Only one real conflict, in run()'s loop -- the same class as prior syncs: HEAD's preemption race vs upstream's plain accept-loop tail. Resolved by taking HEAD's side (a strict superset). The change that needed real attention, not just conflict resolution: Devolutions#1242 makes run/run_connection/run_connection_with return ServerResult (a new typed error), keeping the anyhow-returning bodies as private _inner methods -- exactly the same wrap-and-rename shape Devolutions#1721 already used for run_connection_with (now a third layer: run_connection_with -> run_connection_with_inner [Devolutions#1242, does the Devolutions#1721 static-channels reset] -> run_connection_inner [the real body, PendingConnection-based here]). Upstream's own accept loop switched to calling run_connection_with_inner directly instead of the now-ServerResult-returning public wrapper, since its `conn`-equivalent needs anyhow::Error for on_disconnected and a consistent type across match arms. This branch's preemption race has the identical two call sites (Entry::Fresh's stream, in both the preempt-enabled race and the plain-queue-behind fallback) that were still calling the public run_connection -- which would have silently started returning the wrong error type (ServerResult vs serve_negotiated's anyhow Result, breaking the match arms' common type) had the merge not been caught by an actual build. Switched both to run_connection_with_inner(stream, TransportTls::Managed), matching upstream's own fix exactly, with a comment explaining why (this is not a stylistic change -- it's required for the code to typecheck at all once the two arms feeding into `conn` must share one Result type). Devolutions#1417 (USB) and Devolutions#1773 (RDPEI)'s server.rs footprint is additive and, for USB, feature-gated (`#[cfg(feature = "usb")]`) so it's a no-op in every build this branch's own verification exercises; Devolutions#1754 relocated the autodetect unit tests out of this crate into ironrdp-testsuite-core, so ironrdp-server's own suite drops from 24 to 14 tests -- confirmed by grep that they still exist there, not lost. 14/14 tests pass in both default and --features egfx (down from 24, accounted for above, not a regression), clippy --all-targets clean in both, fmt clean, full cargo build --workspace green.

…upstream Devolutions#1242/Devolutions#1243/Devolutions#1244) 13 upstream commits since the last sync, the significant one being the staged anyhow -> typed ServerError migration (Devolutions#1242/Devolutions#1243/Devolutions#1244): the module's own `Result` alias (`anyhow::Result`) is gone, replaced by `ServerResult<T> = Result<T, ServerError>`, with `ServerErrorExt` constructors (encode/decode/io/channel/unsupported/reason/custom) and `ResultExt::map_err_kind(context, ServerErrorKind::Variant)` replacing anyhow's `.context()`. Two small textual conflicts (upstream's un-refactored accept_begin/ accept_credssp call sites landing where this branch's calls into PendingConnection::negotiate_and_authenticate/complete_security_upgrade now are) -- resolved by keeping this branch's structure and discarding upstream's inline CredSSP block from finalize_and_shutdown, since that logic already ran earlier here, inside complete_security_upgrade. The real work wasn't in the conflict markers: this branch's own three functions (negotiate_and_authenticate, complete_security_upgrade, finalize_negotiated) don't exist on master, so nothing in upstream's diff converts them for me. Converted all three from Result<...> (dead now that anyhow::Result is gone from scope -- the bare Result<T> became std::result::Result's 2-generic-arg form and stopped compiling outright) to ServerResult<...>, and applied the identical map_err_kind pattern upstream uses for the SAME underlying accept_begin/accept_credssp calls elsewhere in this same file, so the conversion matches house style rather than inventing a new one. Verification catches problems a plain build doesn't (per the Devolutions#1476 sync lesson: #[cfg(test)] doesn't compile under a bare `cargo build`). 4/4 tests pass in both default and --features egfx (was 13; Devolutions#1754 relocated 10 autodetect tests out of this crate, upstream's own run_connection_releases_the_static_channels test adds 1 back -- 13 - 10 + 1 = 4, accounted for, not a loss). clippy --all-targets clean in both configs, fmt clean, full cargo build --workspace green.

…#1243/Devolutions#1244) Two upstream commits since the last sync: Devolutions#1243 (encoder/helper/echo internals to ServerError) and Devolutions#1244 (server.rs internals + on_disconnected to ServerError) -- the deeper half of the staged anyhow -> ServerError migration that Devolutions#1476's prior sync (304eff4) only got the first step of (Devolutions#1242, public API boundary only). Four textual conflicts. Two match Devolutions#1588's identical fix, already merged there (199e6c1): the un-refactored accept_begin/accept_credssp call sites landing where this branch's PendingConnection calls now are, and upstream's plain-loop tail vs this branch's preemption race (took this branch's side, a strict superset). One new: encode_share_data_pdu's call in send_auto_reconnect_cookie collided with upstream's error-style conversion of the SAME site -- kept this branch's 4-arg call (the pdu_source parameter added earlier for the eviction-notice work) with upstream's .map_err(ServerError::io(...)) style instead of the old .context(...). The conflict markers were the easy part. This branch's own functions don't exist on master, so nothing in upstream's diff converts them -- had to apply the same ServerResult/map_err_kind conversion by hand to negotiate_and_authenticate and complete_security_upgrade (matching Devolutions#1588 exactly, since these are the same functions) AND to this branch's own additions that Devolutions#1588 doesn't have: PreemptRace::Ended, serve_negotiated, finalize_negotiated, and the `conn` future's Output type -- all four had to agree on ServerResult<()> since `conn` is built from either run_connection_inner or serve_negotiated and PreemptRace::Ended carries whichever one resolves. Also: upstream's Devolutions#1244 folded run_connection_with_inner back into run_connection_with (per its own description), leaving only run_connection_inner as the pure per-connection body with no static-channels reset -- exactly what this branch's two preemption call sites need (the race's `conn` and the plain-queue-behind fallback), but they were still calling the now-nonexistent run_connection_with_inner. Renamed both call sites to run_connection_inner. One test-only fallout, caught by `cargo test` (a plain `cargo build` doesn't compile #[cfg(test)] code, per the earlier Devolutions#1476 sync lesson): the NoDisplay test mock's updates() implementation relied on this module's bare `Result` alias, which no longer exists now that anyhow is out of scope here -- RdpServerDisplay::updates() itself still returns anyhow::Result (its conversion is Devolutions#1245, not yet landed), so qualified explicitly as anyhow::Result rather than guessing at a ServerResult shape the trait doesn't actually expect yet. 14/14 tests pass in both default and --features egfx (was 24; Devolutions#1754's autodetect relocation already accounted for in the prior sync, so this drop is expected, not new). clippy --all-targets clean in both configs, fmt clean, full cargo build --workspace green.

…s new one (upstream Devolutions#1798 + friends) Six upstream commits since the last sync: Devolutions#1798 (graceful disconnect via ServerEvent::Disconnect(ErrorInfo)), Devolutions#1786 (pointer cache size), Devolutions#1784 (wire RdpdrServer -- additive, same static_channel_factories-style pattern as the two prior syncs, composed cleanly), Devolutions#1796! (drop tokio/tokio_rustls re-exports -- zero server.rs footprint, this file already imports both directly), Devolutions#1734 (bandwidth), Devolutions#1245! (display traits to ServerResult, drop anyhow dep entirely). Three conflicts, all in the same area: Devolutions#1798 adds ServerEvent::Disconnect(ErrorInfo) -- a general-purpose "disconnect with a caller-chosen reason" event -- landing right where this branch's own ServerEvent::EvictedByOtherConnection (a preemption-specific "always DisconnectedByOtherconnection" event) already lives. Different mechanisms serving different purposes, so kept BOTH variants, both Debug arms, and both dispatch_server_events handlers rather than picking one. The one call site that needed more than keeping both: Devolutions#1798's own Disconnect handler calls encode_share_data_pdu(pdu, io_channel_id, user_channel_id) -- the pre-existing 3-arg shape, since Devolutions#1798 predates this branch's pdu_source parameter (added earlier for the eviction notice, after an automated review caught that MS-RDPBCGR 2.2.5.1.1 requires pduSource=0 for TS_SET_ERROR_INFO_PDU specifically, not user_channel_id). Devolutions#1798's Disconnect handler sends the exact same PDU type (ServerSetErrorInfoPdu) for the exact same reason (a graceful disconnect-with-explanation), so the identical fix applies: changed its call to encode_share_data_pdu(pdu, 0, io_channel_id, user_channel_id), matching the EvictedByOtherConnection arm right above it. This is a correctness fix carried into upstream's own new code during the merge, not just a mechanical adjustment to keep it compiling. Devolutions#1245 flipped RdpServerDisplay::updates() from anyhow::Result back to ServerResult (its own migration, deferred at the PRIOR sync when anyhow was still in scope for that one trait) -- the NoDisplay test mock's anyhow::Result qualification from that prior sync is now wrong and reverted to plain ServerResult, matching the trait again. New clippy warning at the merged line for ErrorInfoDisconnectHandle:: disconnect ("Err-variant is very large") confirmed PRE-EXISTING on a fresh plain-master clone at the same function (different line number only) -- not introduced by this merge, left alone. 14/14 tests pass in both default and --features egfx (unchanged from the prior sync), clippy --all-targets clean in both configs (module the pre-existing upstream warning above), fmt clean, full cargo build --workspace green.

…disconnect Checks [linux] and Checks [macos] both failed on the same underlying issue: CI runs cargo clippy with --features helper,__bench -- -D warnings, a stricter combination than this branch's own verification had exercised (-p ironrdp-server --all-targets, default and --features egfx only). Under that combination, clippy::result_large_err on ErrorInfoDisconnectHandle::disconnect (upstream Devolutions#1798, merged into this branch's last sync, 0436580) escalates from a warning to a hard error. Confirmed via direct reproduction, not assumption, on two fronts: (1) this exact scoped clippy invocation fails identically on a fresh plain clone of upstream/master -- so the lint itself is a pre-existing bug in Devolutions#1798, not something this merge introduced; (2) the sibling method, AutoReconnectCookieHandle::set, already carries a #[expect( clippy::result_large_err, ...)] suppression for the identical SendError<ServerEvent> return type, which Devolutions#1798 evidently didn't know to copy when adding its own analogous handle. Mirrored that exact suppression onto disconnect(). Verified the copied reason text is actually true for this call site, not just copy-pasted: a temporary size_of probe (added, checked, then removed) confirmed ServerEvent is 128 bytes with RdpdrServerMessage (also 128 bytes) as the driver, while ErrorInfo itself is only 8 bytes -- so "ServerEvent's size is driven by its largest per-channel payload (RdpdrServerMessage), not by anything this method does" holds for disconnect() exactly as it does for set(). Re-ran CI's exact command after the fix: cargo clippy -p ironrdp-server --all-targets --features helper,__bench --locked -- -D warnings is now clean (only the pre-existing MSRV-mismatch note, duplicated across every crate, unrelated). This branch's own standard verification also re-run clean: 14/14 tests in both default and --features egfx, clippy --all-targets clean in both, fmt clean, full cargo build --workspace green.

…volutions#1799, Devolutions#1812) Two upstream commits since the last sync: Devolutions#1812 fixed the exact result_large_err lint I'd already fixed on this branch last turn -- identical attribute, identical reason text. Merged with NO conflict: git recognized both sides made the same change to the same region relative to the common ancestor. Devolutions#1799 disables TCP_NODELAY on accepted connections, landing where this branch's preemption race already restructured the plain accept loop (the same conflict class as every prior sync -- resolved by keeping this branch's superset). But this one wasn't safe to resolve by taking HEAD's side and discarding upstream's alone, the way the purely structural instances of this conflict have been: Devolutions#1799 carries a real behavior change (RDP is small latency-sensitive writes; Nagle holds the trailing partial segment of each until the peer's ack, which against delayed-ACK peers is dead time on every write), not just a different shape of the same logic. Upstream's fix has exactly one accept path to patch. This branch has two: the primary listener.accept() (no live session), and the second one inside the preemption race (PreemptRace::Accepted, a candidate connecting WHILE a session is live). A candidate that wins the race becomes the live session exactly like a primary accept does, so it needs TCP_NODELAY too -- applying upstream's fix only where their single code path put it would have silently left every preemption takeover without it. Added the identical set_nodelay(true) + warn-on-failure logic at both accept sites, matching upstream's wording where the comment is describing the same fact (Nagle/delayed-ACK) and adding a line at the second site noting why it needs the same fix. 14/14 tests pass in both default and --features egfx, clippy --all-targets clean in both configs AND under CI's exact stricter command (--features helper,__bench --locked -- -D warnings, the one that caught the result_large_err regression two syncs ago), fmt clean, full cargo build --workspace green.

…arge pointer capset) Three upstream commits since the last sync, all in client_accepted's capability loop or client_loop's body -- code this branch has never touched (the preemption/negotiation work stays entirely within PendingConnection/negotiate_and_authenticate and the accept-loop race, upstream of accept_finalize where client_accepted/client_loop live): Devolutions#1787! (Large Pointer Capability Set) -- additive to the capabilities match in client_accepted and UpdateEncoder::new's constructor, neither touched here. Devolutions#1834 (EGFX flow-control + dispatch timing diagnostics) -- logging only inside client_loop's existing futures, no signature changes. Devolutions#1842 (periodic Heartbeat PDUs) -- adds a field + a config setter to RdpServer and a 5th joined future inside client_loop; client_loop's own signature gained a parameter, but its only two call sites (both inside client_accepted) are upstream's own code, already updated by the merge. One trivial conflict: both branches touched the same two import lines at the top of the file (this branch added IpAddr for the anti-storm cooldown's EvictedPeer.ip; upstream added AtomicU64 for the heartbeat write-counter) -- kept both. Note from Devolutions#1834's own commit message, checked rather than trusted: it warns the workspace-wide test build was already broken on master independent of that PR (an ironrdp-daemon call-site mismatch). Did not reproduce here -- full cargo build --workspace succeeded clean, so either it was already fixed by a later commit or doesn't affect a non-test build; not chased further since it doesn't block this merge. 14/14 tests pass in both default and --features egfx (test count unchanged -- none of the three commits touch a test module), clippy --all-targets clean in both configs AND under CI's exact stricter command (--features helper,__bench --locked -- -D warnings), fmt clean, full cargo build --workspace green.

Benoît Cortier (CBenoit) · 2026-09-08T00:55:37Z

+    /// Off by default, so a second connection queues behind the live one — the
+    /// historical behaviour, appropriate for a server expecting many
+    /// short-lived connections. Turn this on for a server backing a single
+    /// specific session (e.g. one that mirrors one desktop), where a newly
+    /// connecting client should replace a stale one rather than hang behind it.


"a server expecting many short-lived connections"

How common is that?

Fair challenge — I don't actually have data on how common that deployment shape is among ironrdp-server's embedders, and the doc shouldn't have implied I did.

Reworded in f5572ae to drop the prevalence claim and ground the off-by-default choice in something I can actually stand behind: it's ironrdp-server's pre-existing behavior, kept as the default so an embedder already relying on it isn't surprised by upgrading. That's a compatibility argument, not a market-share one. Fixed the duplicate copy of this doc on the RdpServerOptions field too, same wording issue.

🤖 Addressed by Claude Code

Anton Mostovoy (@antonmos) Asking to the human, what do you think should be the default value ultimately? We can always document or find a way to communicate that the default will change ultimately if that’s the more sensible default.

Personally, i would default to the new behavior because it's least surprising to end users.

"a server expecting many short-lived connections"

How common is that?

i think an example would be a DDOS attack. Probably not likely since RDP shouldnt be exposed to internet.

Agreed. I’ll merge this PR as-is, but could you open a follow up PR to address this? Thank you!

Follow-up PR opened: #1934 — defaults preempt_existing_session to true (the option is unreleased, so no published default changes). I flagged one thing for review there: under Tls/None the new default means an unauthenticated peer can preempt out of the box (the existing per-mode warning fires by default); only Hybrid gates takeover on client auth.

… prevalence CBenoit asked "how common is that?" of the doc's claim that queue-behind (the off-by-default state) is "appropriate for a server expecting many short-lived connections" -- a fair challenge. I have no data on how common that deployment shape actually is among ironrdp-server's embedders, and the doc shouldn't assert one. Reworded both copies (the field doc in server.rs and the builder setter's doc, which duplicates it) to ground the off-by-default choice in what's actually defensible: it's ironrdp-server's PRE-EXISTING behaviour, kept as the default so an embedder already relying on it isn't surprised by upgrading. That's a compatibility argument, not a market-share claim, and needs no evidence about which usage pattern is more common. Doc-only change. 14/14 tests pass in both default and --features egfx, clippy --all-targets clean in both configs and under CI's exact --features helper,__bench --locked -- -D warnings, fmt clean, full cargo build --workspace green.

Benoît Cortier (CBenoit)

LGTM

…nnection-negotiation * upstream/master: (124 commits) feat(server): let an authenticated connection preempt an existing session (Devolutions#1476) ci(pr-automation): recover reviewer stages (Devolutions#1922) ci(pr-automation): report review check diagnostics (Devolutions#1926) ci(pr-automation): recover agent runtime (Devolutions#1921) ci(pr-automation): report reviewer recovery (Devolutions#1920) docs(pr-automation): define review recovery intent (Devolutions#1915) feat(web): expose enable_server_pointer from the WASM module (Devolutions#1914) fix(web): restore default remote cursor rendering (Devolutions#1910) feat(usb)!: support SuperSpeed packet sizes (Devolutions#1883) ci(pr-automation): explain reviewer ineligibility (Devolutions#1906) ci(pr-automation): explain skipped reviews (Devolutions#1907) ci(pr-automation): request JSON mode (Devolutions#1908) ci(pr-automation): repair empty agent responses (Devolutions#1905) ci(pr-automation): diagnose invalid repairs (Devolutions#1904) ci(pr-automation): simplify review findings (Devolutions#1901) fix(server): bound accept_finalize so a wedged client cannot hold the server (Devolutions#1890) ci(pr-automation): improve reviewer diagnostics (Devolutions#1898) ci(pr-automation): redispatch explicit retries (Devolutions#1897) test: focus PR automation workflow coverage (Devolutions#1896) feat(agent): add clipboard image support (Devolutions#1877) ... # Conflicts: # crates/ironrdp-server/src/server.rs

Devolutions#1476 added `preempt_existing_session: bool` to answer "what should `run` do when a second connection arrives while a session is live?". A bool answers it with two states, but there are three: leave the newcomer in the backlog, close it immediately, or let it take over. A bool cannot grow the third without changing type at every call site. `ConnectionPolicy { Queue, Reject, Preempt }` carries all three on one knob: - `Queue` (default) is the pre-existing backlog behaviour. - `Preempt` is Devolutions#1476's authenticated takeover, machinery unchanged -- only the option that selects it moves from the bool to this variant. - `Reject` closes the extra connection immediately: `run` polls the session and the listener together and drops any newcomer, so a second client fails fast instead of hanging. The session arm is biased first, so a client reconnecting the instant a session ends is served, not rejected. `with_preempt_existing_session(bool)` becomes `with_connection_policy(ConnectionPolicy)`; `Queue` stays the default, so no existing caller changes behaviour. The preemption security table moves to the `Preempt` variant's docs. Reject is the policy a downstream single-session server (e.g. hypr-rdp, Devolutions#8) otherwise reimplements around `run_connection`; a builder option lets it drop that and adopt this instead.

`RdpServer::run` serves one connection at a time: while a session runs it awaits `run_connection` inside the accept `select!`, so it does not call `accept()` again and a second client sits unanswered in the listen backlog until the first ends. To that client the connection appears to hang. This is the behaviour #1483 raises. `ConnectionPolicy` lets the caller choose what happens to that second connection: - **`Queue`** (default) keeps today's behaviour — the extra connection waits in the backlog. - **`Reject`** closes it immediately. While a session runs, `run` now polls the session and the listener together and drops any connection that arrives, so the second client fails fast and can retry rather than hanging. The running session is never interrupted: the session arm is polled first (`biased`), so a client reconnecting the instant a session ends is taken by the outer loop, not rejected. This is the reject half of the policy discussed in #1483. It deliberately leaves authenticated takeover — serving a new client while the incumbent still holds the slot — to a later `Preempt`, without foreclosing it: the `else` branch stays inline where #1476/#1588 would add the negotiation race. A downstream consumer (hypr-rdp) currently reimplements exactly this reject loop around `run_connection` because there was no way to ask `run` for it; a policy on the builder lets it drop that and adopt the upstream one with a single line. Set via `RdpServerBuilder::with_connection_policy`. ### Tested Two tests over a real listener in `tests/server/connection_policy.rs`: with `Reject` the second connection is closed during a session; with `Queue` it is left waiting. Both drive `run`'s accept loop end to end.

…-vendor trap Devolutions/IronRDP#1476 ("let an authenticated connection preempt an existing session") MERGED 2026-09-08 by CBenoit, merge commit 5198cde0. That is the upstream form of vendored server divergence (23), so it drops at the next pin bump. macrdp pins IronRDP by git rev, so no crates.io release is needed — any rev at-or-after 5198cde0 carries it. Verified against the merge commit: all five tunables are identical to this fork's, constant for constant (EVICTION_GRACE 750ms, REPREEMPT_COOLDOWN 5s, REPREEMPT_MAX_LOCKOUT 30s, CANDIDATE_NEGOTIATION_TIMEOUT 10s, CANDIDATE_HANDOFF_GRACE 750ms), and every load-bearing fn is present (upstream's discard_stale_session_events is this fork's discard_stale_eviction_events). So the bump is an adoption, not a re-port. The trap, recorded because it is the #179 class mirrored: upstream defaults preempt_existing_session to false, while this fork preempts unconditionally — there is no option, so the builder chain in src/main.rs has no preemption call. Deleting divergence (23) without adding .with_preempt_existing_session(true) silently reverts second-client takeover to the pre-#174 hang, with no compile error to catch it. Also notes that PR #1588 (the prerequisite refactor) was left open and is now superseded, its extraction having landed inside #1476. Docs only — no code change.

@maryny4

… ConnectionPolicy The note added in 70de029 told a future pin bump to call `.with_preempt_existing_session(true)`. That method no longer exists. Devolutions/IronRDP#1476 shipped a `preempt_existing_session: bool` option on 2026-09-08. Three days later IronRDP#1913 (@maryny4, merged 2026-09-11, f7732a16) removed it and replaced it with an enum: ConnectionPolicy { Queue, Reject, Preempt } // default: Queue RdpServerBuilder::with_connection_policy(ConnectionPolicy) `Preempt` keeps #1476's full-auth gating, and all five preemption tunables are still identical to this fork's — re-checked against both the merge commit and current upstream master. The trap is unchanged, only renamed: the default is `Queue`, macrdp preempts unconditionally, so deleting divergence (23) without adding `.with_connection_policy(ConnectionPolicy::Preempt)` silently reverts second-client takeover to the pre-#174 hang. Copying the old method name from a stale note now fails to compile, which is the safe failure; omitting the call entirely is still the dangerous one, and the note now says so. Also records that the bump is NOT purely drop-in. Upstream's Preempt (present since the #1476 merge) calls invalidate_auto_reconnect_cookie_on_eviction() after an eviction; this fork has no equivalent. Matching constants and functions was necessary but not sufficient. It is likely an improvement, but it sits on the ARC reconnect path REPREEMPT_MAX_LOCKOUT's reclaim-after-link-drop case depends on, so the note asks for a live reclaim test at the bump and a check that the invalidation targets the stale session's cookie rather than the winner's fresh one when both are the same client. Docs only.

Copilot AI review requested due to automatic review settings July 26, 2026 17:34

Copilot started reviewing on behalf of Anton Mostovoy (antonmos) July 26, 2026 17:34 View session

Copilot AI reviewed Jul 26, 2026

View reviewed changes

Comment thread crates/ironrdp-server/src/server.rs Outdated

Anton Mostovoy (antonmos) mentioned this pull request Jul 26, 2026

fix(server): a second client hangs instead of taking over the live session clintcan/macrdp#174

Merged

github-actions Bot added A-community size/L Size: up to 899 counted lines and 20 files; exceeds M in either measure labels Jul 31, 2026

Benoît Cortier (CBenoit) added risk/medium Behavioral change that does not substantially alter a core public API ai-reviewed/1 One automated review completed labels Aug 3, 2026

Anton Mostovoy (antonmos) force-pushed the feat/preempt-existing-session branch from bbba4c9 to e390f81 Compare August 3, 2026 17:54

github-actions Bot added the needs-review A human reviewer is the current next actor label Aug 3, 2026

Anton Mostovoy (antonmos) changed the title ~~feat(server): let a new connection preempt an existing session~~ feat(server): let an authenticated connection preempt an existing session Aug 3, 2026

Benoît Cortier (CBenoit) removed rust labels Aug 4, 2026

Anton Mostovoy (antonmos) mentioned this pull request Aug 8, 2026

refactor(server): separate connection negotiation #1588

Closed

Anton Mostovoy (antonmos) added 3 commits August 18, 2026 09:38

MuNeNiCK (MuNeNiCK) mentioned this pull request Aug 25, 2026

Reconnection hangs behind a non-progressing incumbent RDP session MuNeNiCK/hypr-rdp#53

Closed

Anton Mostovoy (antonmos) added 4 commits August 27, 2026 13:32

Anton Mostovoy (antonmos) mentioned this pull request Sep 3, 2026

fix(server): bound accept_finalize so a wedged client cannot hold the server #1890

Merged

Marynych Oleksandr (maryny4) mentioned this pull request Sep 5, 2026

feat(server): add ConnectionPolicy to reject a second connection #1913

Merged

Benoît Cortier (CBenoit) reviewed Sep 8, 2026

View reviewed changes

Comment thread crates/ironrdp-server/src/builder.rs

Benoît Cortier (CBenoit) approved these changes Sep 8, 2026

View reviewed changes

Benoît Cortier (CBenoit) merged commit 5198cde into Devolutions:master Sep 8, 2026
41 checks passed

Anton Mostovoy (antonmos) mentioned this pull request Sep 9, 2026

feat(server): default ConnectionPolicy to Preempt under Hybrid #1934

Merged

This was referenced Sep 15, 2026

RdpServer::run serves one connection at a time — a second client silently hangs #1483

Closed

ConnectionPolicy::Preempt drops ConnectionHandler hooks fired during the served connection (on_connection_info never fires) #1969

Closed

Greg Lamberson (glamberson) mentioned this pull request Sep 20, 2026

ironrdp-server: introduce a connection-scoped state struct #1978

Open

Conversation

Anton Mostovoy (antonmos) commented Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Security model

Shape

Eviction notice

Testing

Related

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

clintcan commented Jul 29, 2026

Uh oh!

Benoît Cortier (CBenoit) commented Aug 3, 2026

Uh oh!

clintcan commented Aug 3, 2026

Uh oh!

Anton Mostovoy (antonmos) commented Aug 3, 2026

Uh oh!

Anton Mostovoy (antonmos) commented Aug 3, 2026

Uh oh!

clintcan commented Aug 3, 2026

Uh oh!

Anton Mostovoy (antonmos) commented Aug 8, 2026

Uh oh!

Benoît Cortier (CBenoit) Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

Anton Mostovoy (antonmos) Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

Anton Mostovoy (antonmos) Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

Anton Mostovoy (antonmos) Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

Anton Mostovoy (antonmos) Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Benoît Cortier (CBenoit) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Anton Mostovoy (antonmos) commented Jul 26, 2026 •

edited

Loading