refactor(server)!: split per-connection state - #2059
uchouT (uchouT) wants to merge 2 commits into
Conversation
|
This pull request may overlap with #2048. Both PRs touch ironrdp-server connection lifecycle to make auto-detect per-connection: each creates a fresh AutoDetectManager per connection and resets the RTT, baseline-RTT and bandwidth handles to u32::MAX (this PR additionally advances the bandwidth generation). This PR goes further with a ConnectionState struct holding channels, USB and UDP state, but the shared scope is the per-connection reset of auto-detect state. This notice is advisory only. Automated review continues as usual, and how these pull requests relate is for maintainers and authors to decide. Note LLM-assisted content (no human feedback). |
There was a problem hiding this comment.
The PR moves per-connection state (static channels, heartbeat flag, AutoDetectManager, URBDRC allocator/router) from RdpServer fields into a ConnectionState created in finalize_negotiated and dropped on every exit path. Both entry paths funnel through finalize_negotiated (server.rs:2068, 2200), so the deleted manual resets in run_connection_with and the run-loop completion handler are covered structurally; the USB factory/borrow-splitting and heartbeat-flag plumbing check out; the release-then-Acquire ordering on the bandwidth-generation reset matches the documented contract. No wire-protocol or correctness defect found. The three removed public APIs have no remaining in-repo callers. Remaining publishable items are low severity: a deleted #1721 regression test leaves no lifecycle-level coverage of channel-backend release, min/max/avg RTT statistics are now computed but unreachable by embedders while the enable_autodetect doc redirects to handles that do not expose them, and the bandw…
- [skeptical]
#1721 regression test for static-channel backend release deleted without replacement — low 🟡 — crates/ironrdp-server/src/server.rs
The deleted test was the executable guard for the#1721 leak (channel backends such as rdpsnd audio capture held open until the next client). The new ConnectionState local to finalize_negotiated makes the release invariant compiler-enforced on all return paths, so the mechanism deletion is defensible, but no remaining test verifies end-to-end that resource-owning backends are dropped when a connection ends, including via serve_negotiated (the preemption path the removed run-loop reset comment explicitly called out). A cheap lifecycle-level test (Drop-flag channel backend released after run_connection_with on an early-EOF stream) would preserve that coverage.
| /// Send probes via [`ServerEvent::AutoDetectRttRequest`] and read the | ||
| /// results through [`Self::autodetect_rtt_handle`], | ||
| /// [`Self::autodetect_baseline_rtt_handle`] and | ||
| /// [`Self::autodetect_bandwidth_handle`]. Each connection measures its own | ||
| /// network path, starting from scratch. |
There was a problem hiding this comment.
[skeptical] min/max/avg RTT statistics no longer reachable by embedders; enable_autodetect doc redirect omits the gap — low 🟡 — Removing rtt_snapshot() is justified for its stale between-connection values, but it was the only path to RttSnapshot's min/max/avg over the RTT window. Verified in the head tree: AutoDetectManager::snapshot() is pub yet the manager now lives only in the private ConnectionState, and the still-pub RttSnapshot type can no longer be produced by any embedder-facing API. The three handles the rewritten enable_autodetect doc points to publish only latest RTT, session-lowest RTT, and bandwidth. If the statistics loss was intended, the doc should say so; otherwise a per-connection stats read path is needed.
| /// Inject a shared handle that increments every time the bandwidth figure | ||
| /// is republished: when a Bandwidth Measure transaction completes, whether | ||
| /// or not it produced a usable figure, and when a new connection resets | ||
| /// the figure to `u32::MAX`. |
There was a problem hiding this comment.
[code-compressor] Bandwidth-generation contract restated in three doc comments that must be edited in lockstep — low 🟡 — This PR adds the same 'increments every time the bandwidth figure is republished ... and when a new connection resets the figure to u32::MAX' sentence to three docs: with_autodetect_bandwidth_generation_handle (builder.rs:424-427), the autodetect_bandwidth_generation field doc (server.rs:753-756), and autodetect_bandwidth_generation_handle (server.rs:1842-1845). The copies already differ in wording (only the field doc cross-references the None case). Keeping the full contract once at the canonical field doc and reducing the builder/method docs to a one-line summary plus a rustdoc link would avoid three coordinated edits whenever the counter's trigger points change. Purely optional, no behavior impact.
|
Thank you for taking this step, and for keeping it to state ownership. It is the first step from #1978 as we agreed there, and the split matches the partition I described. Leaving out display_suppressed, the auto-detect handles and the event channel is the right scope for now, since they belong with the updates() context step. On the auto-detect handles, advancing the bandwidth generation counter on reset is correct. An embedder that rereads the bandwidth only when the generation moves would otherwise keep the previous connection's figure. lamco-rdp-server passes the generation with the value to its bandwidth estimator, so it depends on this. Some per-connection state from my open PRs would belong in ConnectionState. #2046 adds auto_reconnect_issued, which is per connection by design. #2034 drops the previous session's queued events before every connection. #1954 keeps a pending UDP accept task and transport that are orphaned across a Deactivation-Reactivation pass today, which is the activation-scoped state #1978 describes. As I said on #1978, I will rebase my PRs onto your struct once it lands. #2048 makes the same auto-detect reset as this PR, so I am closing it in favour of this one. |
The connection state now lives in a `ConnectionState` created in `finalize_negotiated`, which both entry paths reach, and passed down to everything that serves the connection. It is dropped when the connection ends. BREAKING CHANGE: `RdpServer::get_svc_processor`, `RdpServer:: get_channel_id_by_type` and `RdpServer::rtt_snapshot` are removed. `RdpServer` can only be borrowed from outside between connections, when the first two found no channels and the last returned the previous connection's figures. Auto-detect results are available through `RdpServer::autodetect_rtt_handle`, `autodetect_baseline_rtt_handle` and `autodetect_bandwidth_handle`. Signed-off-by: uchouT <i@uchout.moe>
782d5f9 to
15e6c98
Compare
|
Resolved the conflict introduced by #1954 , moved connection level field into |
There was a problem hiding this comment.
The PR moves per-connection state (static channels, heartbeat capability flag, auto-detect manager, USB manager, Soft-Sync/multitransport gating, UDP accept abort handle, early tunnel payloads) from RdpServer fields into a ConnectionState created and dropped in finalize_negotiated, which both entry paths (run_connection_inner and serve_negotiated) reach, and resets the embedder-facing auto-detect handles plus generation per connection. Verified in pr-head: the refactor is sound, the #1721 channel-release property becomes structural rather than manual, the auto-detect reset advances the generation so cached readers cannot keep the previous connection's figure, and Soft-Sync migration gating semantics are unchanged (no protocol impact). Three maintainability/documentation issues are confirmed in the head tree: the only executable regression test for the #1721 channel-backend release was deleted with no replacement anywhere in the tree, the new per-connection handle reset is documented o…
- [skeptical + code-compressor] Regression test for the
#1721 channel-backend release deleted with no replacement — medium 🟠 — crates/ironrdp-server/src/server.rs
The PR deletes run_connection_releases_the_static_channels (the ResourceChannel/Drop test guarding the#1721 leak where static-channel backends, e.g. rdpsnd audio capture, stayed live until the next client) along with its mod tests block, because it poked the now-private static_channels field. A tree-wide search finds no replacement: the new autodetect test covers only handle resets, and no test exercises backend Drop on the run_connection_with or preemption serve_negotiated path. The release guarantee is now asserted only in comments on ConnectionState and finalize_negotiated, so a future edit that moves channel state out of ConnectionState or adds a path bypassing finalize_negotiated would silently reintroduce the leak. The property stays testable via a static_channel_factory that registers a Drop flag; the PR should carry a replacement regression test. - [skeptical] New per-connection auto-detect handle reset documented only on private fields — low 🟡 — crates/ironrdp-server/src/server.rs
finalize_negotiated now resets autodetect_rtt, autodetect_baseline_rtt and autodetect_bandwidth to u32::MAX and bumps autodetect_bandwidth_generation on every connection (server.rs 2314-2324) — a deliberate change to what embedders observe through the public handles. Only the private-field docs describe it. The public docs still say 'u32::MAX until the first measurement completes' (autodetect_bandwidth_handle, 1911-1917), describe only the Release-after-store pairing (generation handle, 1922-1931), and still call baseline RTT 'session-lifetime lowest RTT' that 'never rises' (1898-1906), which is now per-connection. The builder docs for with_autodetect_bandwidth_handle / with_autodetect_bandwidth_generation_handle (builder.rs 415-435, touched by this PR) omit the reset and the generation-handle doc even lost its description of what increments the counter. Since the PR's stated purpose is to change cross-connection semantics for exactly these handles, the public contract should state that each connection starts at the sentinel and that the reset advances the generation.
Signed-off-by: uchouT <i@uchout.moe>

Part of #1978: : the state-ownership step discussed there
Per-connection state moves out of
RdpServerfields into aConnectionState, created infinalize_negotiated(both entry paths reach it) and dropped with the connection. It holds the static channels, the heartbeat flag, the auto-detect manager, and the URBDRC interface allocator and request router. The manual resets go away, and the auto-detect baseline RTT no longer carries over between connections.Breaking: removes
RdpServer::get_svc_processor,get_channel_id_by_typeandrtt_snapshot. They can only be called between connections, where they returned nothing or the previous connection's figures.Next: a per-connection context for
RdpServerDisplay::updates(), as discussed.cc Greg Lamberson (@glamberson)