Skip to content

refactor(server)!: split per-connection state - #2059

Open
uchouT (uchouT) wants to merge 2 commits into
Devolutions:masterfrom
uchouT:server-refactor
Open

uchouT (uchouT) wants to merge 2 commits into
Devolutions:masterfrom
uchouT:server-refactor

Conversation

@uchouT

Copy link
Copy Markdown
Contributor

Part of #1978: : the state-ownership step discussed there

Per-connection state moves out of RdpServer fields into a ConnectionState, created in finalize_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_type and rtt_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)

Copilot AI balanced review requested due to automatic review settings September 30, 2026 13:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added breaking-change Includes a breaking change, and requires special scrutiny at the boundaries kind/protocol Affects RDP or related protocol behavior kind/technical-debt Internal cleanup work risk/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny size/L Size: up to 899 counted lines and 20 files; exceeds M in either measure triage/overlap Possible overlap with another pull request; advisory only labels Sep 30, 2026
@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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).

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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…

  1. [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.

Comment on lines +1871 to +1875
/// 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Comment thread crates/ironrdp-server/src/builder.rs Outdated
Comment on lines +424 to +427
/// 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`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@github-actions github-actions Bot added ai-reviewed/1 One automated review completed needs-author-action The pull request author is the current next actor labels Sep 30, 2026
@glamberson

Copy link
Copy Markdown
Contributor

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>
@uchouT

Copy link
Copy Markdown
Contributor Author

Resolved the conflict introduced by #1954 , moved connection level field into ConnectionState

@github-actions github-actions Bot removed needs-author-action The pull request author is the current next actor kind/technical-debt Internal cleanup work labels Sep 30, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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…

  1. [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.
  2. [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.

Comment thread crates/ironrdp-server/src/server.rs
Comment thread crates/ironrdp-server/src/server.rs Outdated
@github-actions github-actions Bot added ai-reviewed/2 Final automated review completed needs-author-action The pull request author is the current next actor and removed ai-reviewed/1 One automated review completed labels Sep 30, 2026
Signed-off-by: uchouT <i@uchout.moe>
@github-actions github-actions Bot added automation-failed Exact-head automated classification or review failed or was unavailable and removed needs-author-action The pull request author is the current next actor labels Oct 1, 2026
@github-actions github-actions Bot added risk/unknown Risk could not be determined automatically; needs maintainer-level scrutiny and removed risk/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny labels Oct 1, 2026
@iconint

iconint commented Oct 2, 2026

Copy link
Copy Markdown

image- [ iCON INT
]

This branch was successfully deployed

1 active deployment
llm-providers — 32f68d6e Deployed Oct 1, 2026 by uchouT via Classify pull request #1555
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-reviewed/2 Final automated review completed automation-failed Exact-head automated classification or review failed or was unavailable breaking-change Includes a breaking change, and requires special scrutiny at the boundaries kind/protocol Affects RDP or related protocol behavior risk/unknown Risk could not be determined automatically; needs maintainer-level scrutiny size/L Size: up to 899 counted lines and 20 files; exceeds M in either measure triage/overlap Possible overlap with another pull request; advisory only

Development

Successfully merging this pull request may close these issues.

4 participants