ironrdp-server: introduce a connection-scoped state struct

[uchouT]

Connection-scoped state (static channels, DVC/USB routing, echo/autodetect bookkeeping, etc.) currently lives on RdpServer and is reset by hand between connections. That makes the state lifetime implicit. It is easy for state from a previous connection to leak into the next one, and it forces client_loop to share &mut RdpServer across select! branches.

On the RdpClient, ironrdp-session::ActiveStage already follows a cleaner model. I'd like to refactor ironrdp-server in the same direction:

• move connection-scoped fields out of RdpServer into a struct created after acceptance and dropped at the end of the connection;
• keep server-scoped state (config, handlers, factories) on RdpServer;
• separate activation-scoped state so that deactivation/reactivation doesn't need to move the static channels through the acceptor.

This would also make #1512 easier to address later. And some other lifecycle mismatch problems can be fixed.

[glamberson]

This looks like the state-separation step of the restructuring discussed on #1357, and it matches the partition I laid out to CBenoit when we talked it through. Checking it against current master, here's where the boundary falls.

Already server-scoped and safe to share: The options, the input handler, the display backend, the channel factories, the credential validator and the connection handler. Per-connection state that lives on RdpServer today is reset by hand. static_channels is cleared by run_connection_with after each session, and display_suppressed is cleared by run_connection_inner at the start of each one, with a comment explaining what breaks if it isn't. The event channel is server-global but carries session events, which is why the preemption path from #1476 needed discard_stale_session_events.

While reading this I found that the auto-detect state isn't reset between connections. The manager is created by enable_autodetect() and nothing in the server resets it, min_rtt_ms only ever ratchets down, and the RTT, baseline RTT and bandwidth handles I added in #1346, #1734 and #1737 are never cleared at connection start. As far as I can tell that turns the session-lifetime baseRTT into a server-lifetime floor on any server that handles connections one after another. I haven't reproduced it, but it's the same class of leak this issue describes.

Those handles are also the awkward part of the split. They reach embedders as Arc atomics injected through the builder, because the display backend is built before any connection exists. Once the state is per-connection they should be too. on_connection_info, which I added in #1691, is already a non-exhaustive per-connection context that fires once per connection, before the display updates stream is created, so it looks like the natural carrier. What's open is whether the display backend should receive them through that hook or as an argument to RdpServerDisplay::updates(), which would be a breaking change to a trait consumers implement.

I'd keep this first step to state ownership and leave the runtime abstraction (spawning, select, channels) for separate later steps. Once connection state isn't behind &mut self on the shared struct, serving more than one connection from one server also stops being blocked by the signature.

[uchouT]

Hi, thanks for the detailed breakdown. Greg Lamberson (@glamberson)

Upfront caveat: I haven't planned this out in detail. My context on the rest of the server is limited and I'm not aware of the in-flight PRs this would collide with. I opened this issue mainly to record what surfaced while I was working on the urbdrc connection lifecycle.

What's open is whether the display backend should receive them through that hook or as an argument to RdpServerDisplay::updates(), which would be a breaking change to a trait consumers implement.

I'd go with the argument to updates(). There's already a precedent in the trees: UsbRedirDevice::device_added receives RdpUsbDeviceAnnounceInfo, which carries the UsbDeviceHandle.

For context, here's the end state I have in my mind. Rough, and I'd want to hear whether it conflicts with anything planned:

1. A connection-scoped struct created in finalize_negotiated — both entry
   paths converge there (serve_negotiated and run_connection_inner) — owned
   for the duration of the connection and borrowed &mut by accept_finalize
   and client_loop. Dropping it at connection end replaces the manual resets.
   Besides the fields you listed, I'd move the URBDRC request router there.

2. Then a sans-I/O session stage for the server, mirroring what
   ironrdp-client's active_session() drives on the client side, with
   ironrdp-server acting as the driver. With connection state no longer behind
   &mut self on the shared struct, the select! arms can produce outputs and
   a single execution point can perform the writes, which should let both
   Rc<Mutex<&mut RdpServer>> and SharedWriter go away. I'd start it as a
   crate-ready module inside ironrdp-server.

3. server: reactivation drops interleaved virtual-channel PDUs #1512 — the reactivation sequence taking over the reader and dropping frames
   that don't match the expected hint. That needs the server to own the read
   loop and route by channel id first, so it depends on step 2.

[glamberson]

Thanks for the reply, and for laying out the end state. It fits what I found when I checked the boundary against master, and I think the three steps are a sensible order.

On the open question, I agree with an argument to updates(). The device_added and RdpUsbDeviceAnnounceInfo precedent is the right one. I'd suggest passing a single non-exhaustive context struct rather than positional arguments, the same shape on_connection_info already has, so adding to it later isn't another breaking change to the trait. I'd also keep that as its own step after the connection-scoped struct lands. That keeps the first step a pure change of who owns the state, and it means consumers only have to adapt their RdpServerDisplay implementations once. I maintain one of those consumers, so I'm happy to test the new signature on our side when you get there.

Since you mentioned not knowing what's in flight, here is what touches the same ground, all open and in review:

• feat(server): add accept_finalize_with_multitransport driver #1953 and feat(server): wire UDP multitransport into ironrdp-server #1954 add accept_finalize_with_multitransport and wire UDP multitransport into ironrdp-server. That is a second entry into the accept and finalize path, so the new struct would have to be threaded through it as well as accept_finalize.
• feat(server): default ConnectionPolicy to Preempt under Hybrid #1934 (antonmos) threads a client_supports_errinfo flag into client_loop and dispatch_server_events, alongside the existing heartbeat flag. Per-connection capability flags like these look like natural contents for your struct.
• The RTT, baseline RTT and bandwidth handles from feat(server): expose NetworkAutoDetect RTT via a shared handle #1346, feat(server): expose measured bandwidth to the embedder #1734 and feat(server): expose session-lifetime baseline RTT to the embedder #1737 are the ones that would move from the builder to the per-connection context.

None of this should block you. Ours are earlier in the queue, so I'd rather rebase #1951, #1953 and #1954 onto your struct once it exists than have you plan around them. If it helps, I can post the exact functions and signatures each of them touches, so you can see where the seams fall.