refactor(server): share connection negotiation via a free function

[Anton Mostovoy (antonmos)]

Summary

Pure refactor, split out of #1476 at the size-bot's request. No behavior change.

RdpServer::run_connection_with currently 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 — exactly the constraint #1476 (a preempting connection that negotiates concurrently with the live one) runs into.

This extracts that as a free function taking only what it needs:

• negotiate_and_authenticate: everything from accept_begin through the optional Hybrid CredSSP exchange, i.e. up to (but not including) accept_finalize. Returns a NegotiatedTransport<S> enum capturing which of the three finalize behaviours applies (no upgrade / TLS upgrade / already-offloaded TLS), preserving each exactly.
• complete_security_upgrade: the security-upgrade marking plus the CredSSP exchange, generic over the stream so both TransportTls modes share one definition — accept_credssp has a single call site, as on master.

run_connection_with becomes a thin wrapper. finalize_after_upgrade (which used to also perform the upgrade marking and CredSSP, both now moved into the above) splits into finalize_negotiated + finalize_and_shutdown, reflecting its narrower remaining job.

Scope reduced since the first review

An earlier revision also converted the sound_factory / cliprdr_factory / gfx_factory fields to Rc and extracted attach_channels_impl, so a candidate could build channels from cloned factories. Both are gone — review of #1476 showed the premise was wrong: building those backends for a peer that hasn't authenticated is itself undesirable (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 doesn't consume the static channel set until the MCS Connect Initial in accept_finalize, the attach simply happens later there — only the winner attaches, from self. So the candidate needs no borrowed factories.

Consequences: factories are back to Box as on master, and attach_channels is byte-identical to master (verified by diff), which also retires the conditional-gfx_handle fix and the let_unit_value suppression the extraction had made necessary. Diff dropped from +263/−111 to +147/−63.

Relationship to #1476

This is the shared-negotiation piece of #1476, landed separately so that PR reviews as a smaller feature-only diff. #1476 is rebased on this branch and now contains only the preemption-specific code.

Test plan

• cargo build -p ironrdp-server (default and --features egfx)
• cargo test -p ironrdp-server --lib — 13 existing tests pass unchanged in both configs
• cargo clippy -p ironrdp-server --all-targets — no new warnings vs master, both configs
• cargo fmt -p ironrdp-server -- --check — clean
• cargo build --workspace
• attach_channels diffed against master: identical

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Refactors server connection setup to enable reusable negotiation without borrowing the entire server.

Changes:

• Extracts channel attachment and negotiation into free functions.
• Introduces transport-specific finalization.
• Shares channel factories through Rc.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Benoît Cortier (CBenoit)]

question: Is Rc the right primitive over, e.g.: Arc?

[Anton Mostovoy (antonmos)]

Arc would cost atomics without buying anything here, because it wouldn't make RdpServer any more shareable than it already is.

Arc<T>: Send requires T: Send + Sync, and neither dyn SoundServerFactory nor dyn CliprdrServerFactory is Send — SoundServerFactory: ServerEventSender and CliprdrServerFactory: CliprdrBackendFactory + ServerEventSender, none of which carry a Send bound. (GfxServerFactory does require Send, but it's the only one of the three.) So Arc<dyn SoundServerFactory> would be !Send exactly like Rc<dyn SoundServerFactory>.

I checked rather than assumed — compiled an assert_send::<RdpServer>() probe against both revisions:

• master: fails, on (dyn SoundServerFactory + 'static) and (dyn CliprdrServerFactory + 'static)
• this branch: fails, on the same two (now via the Rcs)

So RdpServer was already !Send before this PR and still is; switching to Arc wouldn't change that. It's also consistent with how the crate already works internally — client_loop builds an Rc<Mutex<&mut Self>> and SharedWriter is Rc<Mutex<..>>, i.e. the whole connection-serving path is single-threaded by construction.

Happy to switch to Arc if you'd rather the fields not foreclose a future Send server — but that would need Send bounds added to the factory traits to actually mean anything, which is a public API change and felt out of scope for a refactor PR.

🤖 Addressed by Claude Code

[Anton Mostovoy (antonmos)]

Following up on my own answer: the right primitive turned out to be neither — the sharing isn't needed at all, and both the Rc and the attach_channels_impl extraction are now gone (5e4f46a). Your question is what prompted the re-look, so thanks for it.

Both existed only so a preempting candidate in the follow-up (#1476) could build its channels without holding &mut self for the whole negotiation. Review of that PR knocked the premise out: building the cliprdr/sound/gfx backends for a peer that hasn't 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 (an audio capture device, clipboard ownership).

The fix there was to attach later rather than to share harder. accept_begin stops at the security-upgrade gate, and the acceptor doesn't consume the static channel set until it processes the MCS Connect Initial in accept_finalize — so only the winner attaches, from self, through the ordinary &mut self method. No borrowed factories, so no Rc, and no need for a free-function variant of attach_channels.

Net effect on this PR:

• factories are back to Box, exactly as on master;
• attach_channels is byte-identical to master again (verified by diff), which also retires the conditional-gfx_handle fix and the let_unit_value suppression that the extraction had made necessary — both of which were review findings in their own right;
• what's left is only what feat(server): let an authenticated connection preempt an existing session #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, clippy --all-targets and fmt clean in both. #1476 has been rebased onto this so the two stay consistent.

🤖 Addressed by Claude Code

[github-actions]

Single-file refactor of the ironrdp-server connection-setup path. Behavioral equivalence verified by reading: negotiation/upgrade/CredSSP/finalize ordering, the TLS-failure quiet-abandon path, the no-shutdown Continue path, and the conditional gfx_handle install all match the pre-refactor code. No wire-format, PDU, or public API change (builder and RdpServer::new still take Box). One blocking finding: the Box -> Rc conversion of the three factory fields is unused in this PR, justified only by an unlanded feature, conflicts with the Arc convention used for every other shared field, and discards the explicit Send bound on GfxServerFactory; it should be dropped here and land as Arc with its real consumer. Two non-blocking items: CredSSP is now duplicated across two match arms where it was single-sourced, and the AttachedGfxHandle = () alias plus its unit binding and lint suppression rest on a claim the signature contradicts.

Protocol analysis: partially_accepted — All seven change mappings verified against head and accepted: the sequence accept_begin, TLS accept or offload wrap, mark_security_upgrade_as_done, CredSSP, accept_finalize is unchanged, as is channel registration order, which still precedes accept_begin. The dead Continued branch is confirmed locally: negotiated is built in that scope only from the Tls/Offloaded arms. The three potential discrepancies are accurate but rejected as findings here; all are pre-existing and unchanged by the diff, and the first two are documented as embedder preconditions at server.rs:1184-1211. The note that Rc makes RdpServer !Send is overstated: it already was, since the sound and cliprdr factory traits lack Send.

1. blocking / medium — crates/ironrdp-server/src/server.rs
   The Box -> Rc conversion of the three factory fields has no consumer in this PR. Every use site is `self.<field>.as_deref()` producing `Option<&dyn Trait>` (lines 1116-1119), which `Box` provides identically; no `Rc::clone` appears anywhere in the crate. The field comment justifies the change entirely by an unlanded feature ("a future consumer of this connection-setup path (e.g. a candidate connection negotiating concurrently with the live one)"), which is speculative extensibility carrying real cost today: `Rc` is inconsistent with how this same struct shares every other piece of state (`display`, `handler`, `ev_receiver`, `credential_validator` all use `Arc`), and it silently discards the explicit `Send` bound that `GfxServerFactory` deliberately carries (gfx.rs:30). It also adds a reallocate-and-move per factory at construction via `Rc::from(Box<..>)`. Recommend keeping `Box` here and introducing shared ownership — as `Arc`, matching the surrounding convention — in the PR that actually needs it. The extraction of `attach_channels_impl` and `negotiate_and_authenticate` stands on its own without this.