feat(dvc): add dynamic channel reservation API

[uchouT (uchouT)]

Summary

• Add DynamicChannelReservation for reserving a server-side DVC ID before constructing its processor.
• Expose the reserved channel ID through channel_id().
• Allow committing the reservation with create().
• Leave the reserved ID unused when the reservation is dropped.

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.