feat(dvc): create channels with assigned id

[uchouT (uchouT)]

Summary

• Add create_channel_with for creating dynamic channel with their assigned dynamic channel id.

Motivation

Some dynamic channel processors need their assigned channel ID during construction, for example when creating a backend handle that routes messages to that channel. The existing create_channel() API only exposes the ID after the processor has already been constructed.

cc Marc-Andre Lureau (@elmarco)

[Copilot]

Pull request overview

This PR adds a server-side “reserve then create” workflow for DRDYNVC dynamic channels, enabling processors that need to know their assigned channel ID at construction time (before the channel is registered and the Create Request is emitted).

Changes:

• Introduces DynamicChannelReservation to hold a reserved DVC ID until a processor is provided.
• Exposes the reserved ID via channel_id() and finalizes registration via create(), returning the Create Request SvcMessage.
• Adds DrdynvcServer::reserve_channel() as the public API entry point.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Marc-Andre Lureau (elmarco)]

Review

Thanks for the PR — the motivation is clear and the implementation is clean. A few things to address:

1. type_id_to_channel_id not updated on create()

with_dynamic_channel registers the processor type in type_id_to_channel_id (so get_channel_id_by_type::<T>() works), but DynamicChannelReservation::create() doesn't — it only has access to the VacantEntry, not the DrdynvcServer.

This means get_channel_id_by_type::<T>() will silently return None for channels created via the reservation path. Meanwhile remove_by_channel_id will try to clean up a mapping that was never inserted (harmless but asymmetric).

If this is intentional, it should be documented on reserve_channel() (e.g. "channels created through a reservation are not discoverable via get_channel_id_by_type"). If not, create() needs access to the type-id map — which likely means the reservation should borrow &mut DrdynvcServer instead of just the VacantEntry, or create() should return something that lets the caller register the type afterward.

2. Leaked channel IDs on drop

The doc correctly says dropping leaves the ID unused, but next_channel_id has already been incremented, so the ID slot is permanently lost. With u32::MAX IDs this is fine in practice, but if reservations are created and dropped in a retry loop, the ID space erodes silently.

Worth adding a note that IDs are not reclaimed. Alternatively, deferring the increment to create() would avoid this, though the borrowing gets trickier.

3. Test coverage

There are no tests for the new API. A minimal test exercising the reserve → channel_id() → create() → verify-channel-exists flow would catch regressions.

4. Minor nit

The panic message "dynamic channels reaches u32::MAX" matches the existing one in insert_channel, but both read better as "reached" (past tense). Not a blocker.

[Marc-Andre Lureau (elmarco)]

Claude decided to post the review directly :)

[Marc-Andre Lureau (elmarco)]

lgtm, Benoît Cortier (@CBenoit) ?

[Benoît Cortier (CBenoit)]

This is shaping up well, and the core idea is sound.

I mainly have two requests:

• I would like to bundle unrelated public-contract changes in a separate PR landing first on its own.
• About the ergonomics: the reservation mechanism should switch to an owned handle with a runtime ID so we can tie together the DrdynvcServer with the handle without borrowing, which is just enough. Code is otherwise functionally sound.

[Benoît Cortier (CBenoit)]

issue: type_id_to_channel_id changes are unrelated to the reservation API and change a public contract. Same in create_channel. Those are two separate public-contract shifts riding along with the feature:

• with_dynamic_channel flips duplicate-type resolution from last-wins to first-wins.
• create_channel-created channels become visible to get_channel_id_by_type, where before
  they returned None.

This is arguably a consistency fix, but they belong in their own PR that can be reviewed and reverted independently, especially since the CHANGELOG is generated from commits and this change would otherwise be invisible to downstream consumers.

[Benoît Cortier (CBenoit)]

I’m handling that part.

[Benoît Cortier (CBenoit)]

praise: The move-only token is the right shape: single-use finalization at compile time for free.

[Benoît Cortier (CBenoit)]

suggestion: Drop the borrow, and carry a plain channel id + a server instance id instead.

rationale: The reservation holding &'a mut into the server means the whole DrdynvcServer is exclusively borrowed while a reservation is live. I think that fights the use case: if constructing the processor (or its backend handle) needs to touch the server, you're stuck, and you can't hold more than one reservation.
By switching the handle to a plain value: { server_id: usize, channel_id: u32 }. It holds no borrow, so multiple reservations can be outstanding and the server stays mutable between reserve and create. create then takes &mut DrdynvcServer.

The residual risk of dropping the borrow is finalizing a handle on a different DrdynvcServer than minted it, which silently succeeds without bumping that server's counter and collides later. I do think it’s anecdotal, because we typically don’t deal with multiple DrdynvcServer at once. That being said, we can close this gap at runtime: stamp each server with a unique id from a global AtomicUsize (you can use Relaxed read-write operation; carried as a fiel), copy it into the handle, and assert equality in create using assert_eq!. This converts silent corruption into a loud, deterministic panic and keeps the handle Send/Sync/Debug/storable.

For the idea:

use core::sync::atomic::{AtomicUsize, Ordering};

static NEXT_SERVER_ID: AtomicUsize = AtomicUsize::new(0);

pub struct DrdynvcServer {
    server_id: usize, // <- new field
    /* … */
}

impl DrdynvcServer {
    pub fn new() -> Self {
        Self { server_id: NEXT_SERVER_ID.fetch_add(1, Ordering::Relaxed), /* … */ } // <- get a unique runtime ID
    }
    pub fn reserve_channel(&mut self) -> DynamicChannelReservation {
        DynamicChannelReservation { server_id: self.server_id, channel_id: self.dynamic_channels.reserve() } // <- pass on the server ID to the handle, so they are tied together.
    }
}

pub struct DynamicChannelReservation {
    server_id: usize,
    channel_id: u32,
}

impl DynamicChannelReservation {
    pub fn channel_id(&self) -> u32 { self.channel_id }

    pub fn create<T>(self, server: &mut DrdynvcServer, channel: T) -> PduResult<SvcMessage>
    where T: DvcServerProcessor + 'static {
        assert_eq!(
            self.server_id, server.server_id,
            "reservation finalized on a different DrdynvcServer than the one that minted it",
        );
        // … insert_reserved(self.channel_id, channel) as before …
    }
}

[Benoît Cortier (CBenoit)]

suggestion: Eager allocation is fine.

rationale: Multiple outstanding handles requires assigning distinct ids at reserve time, i.e. bumping next_channel_id eagerly, i.e. a dropped-uncreated reservation skips an id. That's fine: the space is u32 and no session approaches it. Put that rationale in the doc: monotonic, never reused, dropping skips (not reuses) the id, panic on u32::MAX exhaustion. Keep the existing note that channel_id() is provisional until create(). We also don’t really need to deal with exhaustion in practice, and it’s fine to panic by default, the API should reflect that.

[Benoît Cortier (CBenoit)]

suggestion: Rewrite create_channel in terms of the reserve API.

With the borrow gone, this composes cleanly and kills the current duplication between create_channel and the reservation's create():

    pub fn create_channel<T>(&mut self, channel: T) -> PduResult<SvcMessage>
    where T: DvcServerProcessor + 'static {
        self.reserve_channel().create(self, channel)
    }

[Benoît Cortier (CBenoit)]

suggestion: Encapsulate the counter bump behind the DynamicChannelAllocator. Expose reserve() -> u32 (bump + panic-on-max) and insert_reserved(id, channel) (vacant-or-panic), so create() doesn't reach through two layers of fields. The checked_add-discard oddity in entry_channel() disappears too.

[Benoît Cortier (CBenoit)]

2. Leaked channel IDs on drop

The doc correctly says dropping leaves the ID unused, but next_channel_id has already been incremented, so the ID slot is permanently lost. With u32::MAX IDs this is fine in practice, but if reservations are created and dropped in a retry loop, the ID space erodes silently.

Worth adding a note that IDs are not reclaimed. Alternatively, deferring the increment to create() would avoid this, though the borrowing gets trickier.

This comment was against a diff I didn’t review, but I think my redesign suggestion is reintroducing something similar: don’t reclaim skipped IDs, because monotonic, never-reused IDs are a deliberate safety property to avoid ABA holes, and the u32 space is effectively inexhaustible for a per-session allocator: exhausting the space needs ~4.3 billion reserve-drop cycles. Even a pathological no-backoff retry loop burning thousands of failed reservations a second would need hours-to-days of continuous churn in a single session to bite, which is arguably a caller bug. The realistic version of "retry loop" is tamer than it sounds: the fallible step is processor construction before create (build the processor from channel_id(), then finalize), and retry loops are should be bounded and backoff exponentially. I don’t expect more than a handful of IDs to be consumed at worse before the retry loop is stopped.

[Benoît Cortier (CBenoit)]

Review stages found no PR-specific correctness issue. The protocol concern about a capabilities guard matches pre-existing create-channel behavior and is not a regression introduced here; the change remains low risk with technical-debt classification.

[github-actions]

Single-file change to the DRDYNVC server manager adding a two-phase channel-ID reservation API. No wire format or constant changes; the emitted DYNVC_CREATE_REQ matches the existing create path. My main objection is justification, not mechanics: the new public type, public method, and process-global AtomicUsize identity counter have no in-tree caller, no tests, and no stated use case, in a Core-Tier no_std API-boundary crate — a closure-based `create_channel_with(|id| ..)` would meet the apparent need without a detachable token, a global static, or a cross-server panic. The new path also registers `type_id_to_channel_id` with `or_insert` while `create_channel` registers nothing and `with_dynamic_channel` overwrites; the diff defers this via TODO, at the cost of a live channel losing its mapping when a same-type sibling closes. Two new panic paths land in a function that already returns `PduResult`.

Protocol analysis: partially_accepted — Kept, refined: provisional-ID exposure (#3) and the never-reuse doc (#2). I verified no path returns an ID to the allocator, so the reuse deviation predates the diff; my finding is scoped to freezing it as a public contract. Rejected as standalone: the caps-exchange gap (#1) is not introduced here — `create_channel` behaves identically and caps state was never tracked — so I folded it into a question about where the gate belongs. I confirmed independently that the emitted CreateRequestPdu and SHOW_PROTOCOL wrapping match the existing path, and that `Creation` is the state the CREATE_RSP handler requires while the caps handler promotes only `Pending`. I did not adopt the handoff's benign read of the TypeId divergence: not protocol-visible, but an observable public-API inconsistency.

1. blocking / high — crates/ironrdp-dvc/src/server.rs:47-93
   The PR adds a public type (`DynamicChannelReservation`), a public method (`reserve_channel`), a new field on a public struct, and a process-global mutable `static NEXT_SERVER_ID: AtomicUsize` to a Core-Tier, no_std, API-boundary crate. Nothing in the repo calls the new API, no test exercises it, and neither the diff nor the docs state the problem it solves; the doc's "preventing ABA issues" is vacuous because the pre-change allocator already never recycled IDs. If the need is the chicken-and-egg case where a processor must know its own ID at construction, `create_channel_with<T>(&mut self, f: impl FnOnce(u32) -> PduResult<T>)` supplies the ID with no detachable token, no global static, no cross-server panic, and no leakable provisional ID. The static also puts hidden process-wide state in a foundational crate purely to improve a panic message, and `AtomicUsize::fetch_add` requires `target_has_atomic`, narrowing buildable no_std targets. This surface should land with its consumer or be deferred until one exists.
2. blocking / medium — crates/ironrdp-dvc/src/server.rs:83-89
   The reservation path records `type_id_to_channel_id` via `or_insert`, while `create_channel` (line 287) records nothing and `with_dynamic_channel` (line 248) overwrites via `insert`. So `reserve_channel().create(..)` and `create_channel(..)` — two public APIs emitting an identical DYNVC_CREATE_REQ — differ observably: only the former makes `get_channel_id_by_type::<T>()` return `Some`. `or_insert` is also newly lossy: create two channels of type `T` through the reservation path, close the first, and `remove_by_channel_id` drops the mapping because it matches the removed ID, so `get_channel_id_by_type::<T>()` returns `None` for the still-open second channel and a caller like the Echo path in crates/ironrdp-server/src/server.rs:1427 silently drops requests. The comment at line 321 says the map "[o]nly matters for pre-registered channels", contradicting writing it here. The TODOs at 83-85 and 293-294 acknowledge the divergence and ship it knowingly. Align the semantics first, or omit the TypeId write so the new path matches `create_channel`.
3. blocking / medium — crates/ironrdp-dvc/src/server.rs:73-76
   `create` returns `PduResult<SvcMessage>` yet enforces its contract with `assert_eq!`, and `insert_reserved` (lines 139-141) adds an `unreachable!`. Both are new panic paths in a Core-Tier library an embedder drives per connection, which CLAUDE.md lists as a critical anti-pattern ("panic-oriented code in production paths without strong justification"). Misuse is plausible: a multi-connection server tracking pending reservations in a shared map, or one that reconnects and rebuilds `DrdynvcServer`, can finalize against the wrong instance and take down the connection task instead of getting an error. Since the function already returns `PduResult`, `Err(pdu_other_err!("..."))` is a one-line change preserving the diagnostic. As written, the reservation-identity machinery only converts a delayed `unreachable!` into an earlier panic; returning an error would make the guard useful rather than merely louder.
4. question / medium — crates/ironrdp-dvc/src/server.rs:300-315
   Two sequencing preconditions are left wholly to the caller, with nothing in-tree showing the intended discipline. First, `channel_id()` publishes an ID before any entry exists in `dynamic_channels` and before any PDU is emitted; MS-RDPEDYC has no provisional-identifier concept, so a caller using it to address data or a close before DYNVC_CREATE_RSP would reference a channel the client never acknowledged. `is_channel_opened` cannot distinguish reserved-but-unregistered from unknown. Second, neither `create` nor `create_channel` consults capability-exchange state, and `DrdynvcServer` stores none, so a Create Request can precede DYNVC_CAPS_RSP (MS-RDPEDYC 2.2.1); `with_dynamic_channel` avoids this by using `Pending` and deferring emission to the caps handler. What is the intended consumer, and is the caps gate meant to live in `DrdynvcServer` or the embedder? Without that, the new path widens an unguarded surface instead of adding one safe by construction.
5. non_blocking / low — crates/ironrdp-dvc/src/server.rs:300-307
   The doc promotes monotonic, never-reused IDs from an implementation detail to a guaranteed public contract ("never reused for the lifetime of the server", "permanently skips its ID"). MS-RDPEDYC 3.3.3.2 recommends reusing a Channel ID when creation failed or the channel closed, and mandates only uniqueness, so the behavior is a deviation from advice, not a MUST, and it predates this diff. The cost is freezing it: recycling later becomes a documented-contract break, and sessions that churn channels keep widening the cbId encoding from one to two to four bytes. The "preventing ABA issues" rationale is unsupported — no path returned an ID to the allocator before this change either. Suggest documenting monotonicity as current behavior rather than a guarantee, and dropping the ABA claim.
6. non_blocking / low — crates/ironrdp-dvc/src/server.rs:150-150
   The `expect` message and the `with_dynamic_channel` panic doc at line 241 are reworded from "reaches" to "reached", unrelated to the feature and worse to read: a `# Panics` clause states a condition ("panics if the number ... reaches `u32::MAX`"), so the present tense was correct. Minor, but it is bundled churn in an otherwise API-adding diff, and it leaves the untouched "reaches" at line 286 inconsistent with the edited ones.

[github-actions]

blocking / high: The PR adds a public type (`DynamicChannelReservation`), a public method (`reserve_channel`), a new field on a public struct, and a process-global mutable `static NEXT_SERVER_ID: AtomicUsize` to a Core-Tier, no_std, API-boundary crate. Nothing in the repo calls the new API, no test exercises it, and neither the diff nor the docs state the problem it solves; the doc's "preventing ABA issues" is vacuous because the pre-change allocator already never recycled IDs. If the need is the chicken-and-egg case where a processor must know its own ID at construction, `create_channel_with<T>(&mut self, f: impl FnOnce(u32) -> PduResult<T>)` supplies the ID with no detachable token, no global static, no cross-server panic, and no leakable provisional ID. The static also puts hidden process-wide state in a foundational crate purely to improve a panic message, and `AtomicUsize::fetch_add` requires `target_has_atomic`, narrowing buildable no_std targets. This surface should land with its consumer or be deferred until one exists.

[uchouT (uchouT)]

I did consider create_channel_with<T>(&mut self, f: impl FnOnce(u32) -> PduResult<T>).

I initially rejected this API shape because processor construction and DVC channel creation have two independent error domains. Making the closure generic over E is not sufficient by itself, because the operation may still produce a PduError after the closure succeeds. The API would therefore need either a nested result such as:

Result<PduResult<SvcMessage>, E>

or an additional public error type combining E and PduError.

The reservation API keeps these two phases and their errors separate, and additionally supports deferred channel construction. The trade-off is that it introduces a larger and more misuse-prone public API.

Benoît Cortier (@CBenoit) Marc-Andre Lureau (@elmarco) Would like to hear your insight.