feat(server): let an authenticated connection preempt an existing session - #1476
feat(server): let an authenticated connection preempt an existing session#1476Anton Mostovoy (antonmos) wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
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.
|
Flagging a security concern with the preemption gate before this lands — and pointing at a fix that already exists downstream. The issue: 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. The suggested 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 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)
added
risk/medium
|
Blocking finding: the preemption probe accepts only the |
|
Agree the 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. |
bbba4c9 to
e390f81
Compare
|
Rewritten on current 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 What it does now. A candidate must complete real negotiation — and CredSSP under
Open question for you: the No duplicated negotiation. clintcan flagged that the downstream implementation duplicates the pre- 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 This interacts with the cookie in a way that may deserve its own look independently of this PR: 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 |
github-actions
Bot
added
the
maintainer-required
Anton Mostovoy (antonmos)
changed the title
|
Heads-up on the red
Every other check passed, including I'd normally just re-run to confirm rather than assert it's flaky, but I don't have permission on this repo ( 🤖 Addressed by Claude Code |
|
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:
No opinion to add on the |
github-actions
Bot
added
risk/unknown
size/XL
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`.
|
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 ( Once #1588 merges, I'll rebase this PR on top of it and trim the diff down to just the feature-specific code: the 🤖 Addressed by Claude Code |
attach_channels wrote self.gfx_handle unconditionally from attach_channels_impl's return value. The pre-refactor code only ever wrote self.gfx_handle = Some(handle) from inside the build_server_with_handle() success arm and never touched the field when that returned None (the build_gfx_handler() fallback branch) -- so a stateful factory that returns Some on one connection and falls back to build_gfx_handler() on a later one would previously leave the old handle in place, not clear it. Only overwrite on Some now, restoring that exact behavior. Spotted by Copilot's review on Devolutions#1588.
…ression Two review findings from the automated reviewer on Devolutions#1588: 1. The six-argument accept_credssp call was duplicated across the Tls and Offloaded arms, differing only in framed.as_mut() vs framed, plus a third arm that had to be unreachable!(). Master had a single call site, so this was a regression against the very "cannot drift between them" property the surrounding docs claim. Move the security-upgrade completion into a small generic helper (complete_security_upgrade) that each tls arm calls once: the exchange has one definition again, and the unreachable arm is gone (unreachable!() count is back to master's). 2. The AttachedGfxHandle alias doc claimed it existed so the signature needs no #[cfg], which was inaccurate -- the signature already cfgs the gfx_factory parameter. Corrected to the real reason (a return type cannot itself carry a #[cfg]). Annotating the binding's type drops the unit_bindings suppression and the `let () = ...` consumption; clippy's let_unit_value still fires under egfx-off, so one narrow expect remains instead of the previous two-lint block. Call order is unchanged from master: accept_begin -> tls accept -> mark_security_upgrade_as_done -> accept_credssp -> accept_finalize -> shutdown.
github-actions
Bot
added
kind/protocol
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The refactor half is sound: `negotiate_and_authenticate` and `attach_channels_impl` single-source three duplicated finalize paths, preserve the `BeginResult::Continue` no-shutdown behaviour, and frame the eviction PDU correctly per MS-RDPBCGR 2.2.5.1. The preemption machinery is not ready. `pending = probe.await` (server.rs:1696) is unbounded, so a peer that connects and stays silent parks `accept_begin` forever; when the live session ends, `run()` hangs with no accept and no event drain, unstoppable by `Quit`. Routing eviction through the server-global `ev_sender` (server.rs:1750) lets the event outlive the 750 ms grace drop and kill the winning candidate instead. Also blocking: the docs unconditionally claim an unauthenticated peer can never evict, contradicted by the mode table for Tls and None; and the re-arming source-IP cooldown permanently locks out an auto-reconnecting client from the host it was evicted from.
Protocol analysis: partially_accepted — Verified against the code. Accepted: the ERRINFO code choice, the disconnect after the notice, the permitted omission of Deactivate All and MCS Disconnect Provider Ultimatum, and the CredSSP-gated Hybrid path. Confirmed the RNS_UD_CS_SUPPORT_ERRINFO_PDU conflict (AcceptorResult exposes no early-capability field), but send_access_denied shares the gap, so it is pre-existing and non-blocking here. Adopted the None-mode doc overstatement. Escalated the Tls pre-credential exposure above 'ambiguous': the docs unconditionally claim an unauthenticated peer can never evict. Refined the 5.5 cookie point as descriptive; the concrete nearby defect is the re-arming same-IP cooldown. Nothing rejected; the two highest-severity defects found are non-protocol.
- non_blocking / low — crates/ironrdp-server/src/server.rs
`ServerEvent` is a public enum without `#[non_exhaustive]`, unlike `RdpServerOptions` (line 235), which carries it and so absorbs the new `preempt_existing_session` field cleanly. Adding the `EvictedByOtherConnection` variant breaks any downstream `match` over `ServerEvent` that is exhaustive. Adding `#[non_exhaustive]` in the same change would prevent the next variant from repeating this.
e390f81 to
c80760d
Compare
github-actions
Bot
added
risk/unknown
and removed
risk/medium
…t needed Answering @CBenoit's "is Rc the right primitive over, e.g. Arc?": it turns out to be neither, because the sharing is not needed at all. Both existed only to let a preempting candidate (the follow-up in Devolutions#1476) build its channels without holding `&mut self` for the whole negotiation. That premise is gone: review of Devolutions#1476 showed building the cliprdr/sound/gfx backends for a peer that has not authenticated is itself wrong -- a port scan would construct and tear down backends alongside the live session's, and those factories may claim exclusive OS resources. Since `accept_begin` stops at the security-upgrade gate and the acceptor does not consume the static channel set until it processes the MCS Connect Initial in `accept_finalize`, the attach can simply happen later: only the WINNER attaches, from `self`, via the ordinary `&mut self` method. So the candidate path needs no borrowed factories, which means: - `sound_factory` / `cliprdr_factory` / `gfx_factory` go back to `Box`, and the `Rc` question disappears rather than being answered. - `attach_channels_impl` and the `AttachedGfxHandle` alias are removed; `attach_channels` is byte-identical to master again, which also retires the conditional-install fix and the `let_unit_value` suppression that the extraction had made necessary. What remains is the piece Devolutions#1476 actually consumes: `negotiate_and_authenticate` + `NegotiatedTransport` + `complete_security_upgrade`, and the `finalize_after_upgrade` -> `finalize_negotiated`/`finalize_and_shutdown` split. Diff drops from +263/-111 to +147/-63. Still a pure refactor: 13 existing tests pass under both default and egfx features, clippy --all-targets clean in both, fmt clean.
Four blocking and three non-blocking findings from the automated review, all confirmed against the code before changing anything. BLOCKING/critical -- unauthenticated remote hang of the accept loop. negotiate_candidate blocks on socket reads, and PreemptRace::Ended did a bare `pending = probe.await`. A peer that completed the TCP handshake, cleared on_accept and then sent NOTHING parked accept_begin forever: it disabled accepts for the rest of the session (the accept arm is gated on !probing) and, once the live session ended, hung run() outright with no select! left -- no accepts, no event drain, not even ServerEvent::Quit could stop the server. Two bounds, deliberately separate: CANDIDATE_NEGOTIATION_TIMEOUT (10s) caps the negotiation itself, and CANDIDATE_HANDOFF_GRACE (750ms) caps the post-session-end wait, which is the window where the loop services nothing. A candidate that cannot finish in the grace is dropped and reconnects; holding the listener for it is the worse trade. Regression test included and verified to fail without the fix. BLOCKING/high -- the eviction event could kill the winner. It is queued on the server-global ev_sender but consumed by whichever connection drains it next. An incumbent too wedged to take it within EVICTION_GRACE (being wedged is why it was evicted) left it queued, and the WINNER's client_loop then drained it and disconnected itself, reporting a bogus ERRINFO_DISCONNECTED_BY_OTHERCONNECTION to the client that had just taken over. discard_stale_eviction_events() drops leftovers before serving a winner, putting every other event back in arrival order. BLOCKING/high -- docs contradicted their own table. The prose claimed flatly that an unauthenticated peer can never evict; that holds only under Hybrid. Under Tls the handshake authenticates the SERVER, so any peer that can reach the port clears the bar. Rewrote the security section so the prose matches the table, and added a startup warning under the non-authenticating modes. Deliberately did NOT hard-restrict the feature to Hybrid: that would delete all preemption test coverage, including the on_accept gating test from the previous round, and it is a product call for maintainers -- authenticates_before_eviction() makes it one line. BLOCKING/medium -- the anti-storm cooldown locked out its own use case. refuse_reconnect_from_evicted re-armed on every refused attempt with no cap, so a client whose link dropped could never reclaim its own stale session while auto-reconnecting -- exactly the scenario the docs give as the motivation. Added REPREEMPT_MAX_LOCKOUT (30s) as an absolute cap from the eviction; the ERRINFO notice is the real fix for the loop, this is only a backstop. Test included. non_blocking/medium -- backends were built for unauthenticated peers. negotiate_candidate ran the cliprdr/sound/gfx factories before authenticating, so a port scan constructed and tore down backends alongside the live session's, and those factories may claim exclusive OS resources. accept_begin stops at the security-upgrade gate and the acceptor does not consume the static channel set until it processes the MCS Connect Initial in accept_finalize, so the attach moved into serve_negotiated: only the winner builds backends, still ahead of finalization. NegotiationContext no longer carries the factories at all. non_blocking/low -- recorded the known MS-RDPBCGR 3.3.5.7.1 gap (the Set Error Info PDU is sent without checking RNS_UD_CS_SUPPORT_ERRINFO_PDU, which AcceptorResult cannot currently express) as a code comment, and corrected the None-mode bar from "X.224/MCS negotiation" to a well-formed X.224 Connection Request.
c80760d to
45adf13
Compare
|
Rebased onto the updated #1588, which shrank: the 🤖 Addressed by Claude Code |
Summary
RdpServer::runserves one connection at a time: whilerun_connectionis 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(defaultfalse, 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:HybridTlsCredentialValidatorstill runs later, during finalizationNoneCandidates are additionally gated through
ConnectionHandler::on_acceptbefore 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.Shape
Rather than duplicating the pre-finalization half of
run_connectionfor 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). Bothrun_connection_withand 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.Box→Rcso a candidate holds cheap clones. Builder API unchanged (still takesBox).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:
cargo test,cargo clippy --all-targetsandcargo fmt --checkare clean forironrdp-serverwith and withoutegfx, andcargo build --workspacepasses. The full-auth positive path is exercised downstream against real clients, since it needs a live NTLM/CredSSP client.Related
Design discussion for the broader multi-connection policy is in #1483; this PR stays a single opt-in flag so it doesn't pre-empt that decision.