fix(server): keep ConnectionHandler reachable during a Preempt race

[Anton Mostovoy (antonmos)]

Fixes #1969.

Problem

Under ConnectionPolicy::Preempt, RdpServer::run took the connection handler out of self before building the live connection (that connection borrows &mut self for the whole race, while a candidate's on_accept needs the handler at the same time) and only put it back after the race. Every hook fired from inside the served connection therefore saw None and was skipped with nothing logged — in practice on_connection_info never fired, for every connection under Preempt, not just preempted ones. Queue and Reject were unaffected.

Fix

Share the handler rather than taking it: connection_handler is now Option<Rc<RefCell<Box<dyn ConnectionHandler>>>>. The race clones the Rc for the candidate's on_accept, while the live connection reaches the same handler through self. The take/restore plumbing through the race's return tuple is removed.

Why this shape is safe:

• Every ConnectionHandler method is synchronous, so a borrow_mut() lives only for the call, never across an .await; with the server confined to one thread, the candidate's on_accept and the live connection's hooks can't hold it at the same time.
• RdpServer is already !Send (it holds non-Send factories such as SoundServerFactory), so an Rc field takes nothing away from embedders. The public API (with_connection_handler(Option<Box<dyn ConnectionHandler>>)) is unchanged.

Tests

The three e2e tests from the issue (by clintcan), in ironrdp-testsuite-extra/tests/e2e.rs: a real client through the full handshake via run() under Queue, Reject and Preempt, asserting both on_accept and on_connection_info reach the handler. Without the fix the Preempt case fails with on_accept=true, on_connection_info=false; with it all three pass.

This is the prerequisite Benoît Cortier (@CBenoit) asked for on #1934 (mode-aware Preempt default under Hybrid), which would otherwise make this bug hit every Hybrid embedder out of the box.

🤖 Generated with Claude Code

[Copilot]

Copilot review overview

🟢 Approval recommended

The ownership fix is sound and directly covered by regression tests.

Review effort: Balanced
Findings: None

What changed in this PR

Fixes ConnectionHandler availability during Preempt races without changing the public API.

Changes:

• Shares the handler through Rc<RefCell<_>> during races.
• Adds end-to-end coverage for all connection policies.

No material findings. Protocol review was unnecessary because wire behavior is unchanged.

File	Description
crates/​ironrdp-server/​src/​server.rs	Preserves handler access during preemption.
crates/​ironrdp-testsuite-extra/​tests/​e2e.rs	Tests lifecycle hooks across policies.

[github-actions]

The core fix is sound and minimal: sharing the ConnectionHandler via Rc<RefCell<...>> under Preempt resolves the #1969 borrow conflict without changing the public API (RdpServer was already !Send since it holds non-Send dyn SoundServerFactory), and all four call sites use statement-scoped borrow_mut() consistent with the synchronous trait contract. The take/restore plumbing removal is a strict simplification. I verified the !Send claim, the synchronous trait definition, every converted call site, and that the new test's required imports already exist. The four published findings are all valid, non-blocking observations: the new Preempt e2e test covers only the single-client symptom and never exercises the takeover race where the handler is actually shared; the shared-handler safety invariant is documented but unenforced; the test helper duplicates ~50 lines of existing handshake scaffolding; and three Box::pin wrappers are unnecessary by the file's own precedent.

[github-actions]

Independent review confirms the fix for #1969: under ConnectionPolicy::Preempt the live connection and a candidate's on_accept can now both reach the shared ConnectionHandler. In pr-head, connection_handler is Option<Rc<RefCell<Box<dyn ConnectionHandler>>>>; the take/restore plumbing through the race's return tuple is deleted, and all four former access sites (accept, candidate accept, on_disconnected, on_connection_info) route through the new private with_connection_handler helper, whose RefCell borrow lives only inside a synchronous closure so it cannot span an .await. The !Send justification holds (the server already held non-Send factories) and the public builder API is unchanged; the Negotiated-entry skip preserving on_accept double-count semantics is intact. The e2e tests cover hooks under Queue/Reject/Preempt and a full preemption takeover asserting (2, 2, 1) hook counts plus eviction of the old transport. The single specialist candidate is verified accurate and accepted: a cal…

1. [code-compressor] Touched call site still inlines the address lookup the PR just factored into local_addr_of — low 🟡 — crates/ironrdp-testsuite-extra/tests/e2e.rs
   The PR introduces local_addr_of (oneshot channel + ServerEvent::GetLocalAddr + await/unwraps) and uses it in the new hook tests, and it edits client_server_with_connector to route the handshake through the new connect_client helper. The lines immediately above that edit still carry the identical three-line inline lookup. Replacing them with let server_addr = local_addr_of(&ev).await; is behavior-identical (same send/recv and unwraps, &ev already captured by the surrounding closure) and leaves the address lookup a single home. Low severity: test-only duplication with no correctness or protocol impact. The inlined lines are pre-existing context lines in the diff, not added lines, hence null line bounds.

[Benoît Cortier (CBenoit)]

Thank you, LGTM