Skip to content

fix(rdpsnd-native)!: replace anyhow with typed RdpsndNativeError#1277

Open
Greg Lamberson (glamberson) wants to merge 1 commit into
Devolutions:masterfrom
lamco-admin:fix/rdpsnd-native-typed-error
Open

fix(rdpsnd-native)!: replace anyhow with typed RdpsndNativeError#1277
Greg Lamberson (glamberson) wants to merge 1 commit into
Devolutions:masterfrom
lamco-admin:fix/rdpsnd-native-typed-error

Conversation

@glamberson
Copy link
Copy Markdown
Contributor

Summary

Replaces anyhow::Result on DecodeStream::new with a typed
RdpsndNativeError, bringing the crate into line with the workspace
convention from ARCHITECTURE.md that libraries provide concrete
error types rather than anyhow.

Approach

  • New error module defines RdpsndNativeErrorKind with four variants
    (UnsupportedFormat, OpusInit, AudioDevice, StreamBuild).
    Display and core::error::Error impls are hand-rolled, following
    the post-refactor(pdu): hand-roll Display/Error impls and remove thiserror dependency #1264 pattern in ironrdp-pdu.
  • Type alias RdpsndNativeError = ironrdp_error::Error<RdpsndNativeErrorKind>
    keeps the workspace's Error<Kind> shape.
  • DecodeStream::new returns RdpsndNativeResult<Self> instead of
    anyhow::Result<Self>. Foreign error sources (opus2::Error,
    cpal::DefaultStreamConfigError, cpal::BuildStreamError) attach
    via Error::with_source so the cause chain stays reachable through
    core::error::Error::source().
  • anyhow moves from [dependencies] to [dev-dependencies] for the
    cpal example binary.

Alternatives considered

  • thiserror derive: rejected for consistency with the post-refactor(pdu): hand-roll Display/Error impls and remove thiserror dependency #1264
    direction in ironrdp-pdu. ironrdp-rdpsnd-native is not Core Tier
    and could technically use thiserror, but staying hand-rolled
    matches the recently-established workspace pattern.
  • Wrap source errors inside the kind enum (the ConnectorErrorKind
    style): rejected because the source types (opus2::Error, multiple
    cpal errors) would leak into the public Kind surface for
    marginal matchability benefit.
  • Keep anyhow internally, change only the public signature:
    rejected because anyhow::Error would still propagate through the
    source chain to consumers.

Trade-offs accepted

The four-variant categorisation is coarser than per-error-type
matchability would offer. Consumers needing finer detail traverse the
source chain via core::error::Error::source(). This matches the
existing pattern in PduErrorKind.

Verification

  • cargo xtask check fmt -v green
  • cargo xtask check lints -v green
  • cargo xtask check tests -v green
  • cargo xtask check typos -v green
  • cargo xtask check locks -v green
  • cargo check -p ironrdp-rdpsnd-native --no-default-features green
  • cargo check -p ironrdp-rdpsnd-native --examples green

References

Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates ironrdp-rdpsnd-native to expose a concrete, typed error (RdpsndNativeError) instead of returning anyhow::Result from DecodeStream::new, aligning the crate with the workspace convention of library crates providing typed error surfaces.

Changes:

  • Introduces RdpsndNativeErrorKind and associated RdpsndNativeError/RdpsndNativeResult aliases.
  • Switches DecodeStream::new (and related call sites) from anyhow-based errors to ironrdp_error::Error<Kind> with optional chained sources.
  • Moves anyhow to dev-dependencies (kept for the example) and updates logging to use e.report().

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
crates/ironrdp-rdpsnd-native/src/lib.rs Exposes the new typed error module/types; keeps anyhow referenced under cfg(test).
crates/ironrdp-rdpsnd-native/src/error.rs Adds the new typed error kind and type aliases.
crates/ironrdp-rdpsnd-native/src/cpal.rs Converts DecodeStream::new to typed errors and attaches sources; updates logging.
crates/ironrdp-rdpsnd-native/Cargo.toml Replaces anyhow dependency with ironrdp-error (std) and moves anyhow to dev-deps.
Cargo.lock Adds ironrdp-error to the lockfile dependency graph.

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

Comment thread crates/ironrdp-rdpsnd-native/src/lib.rs Outdated
Comment thread crates/ironrdp-rdpsnd-native/src/cpal.rs Outdated
@glamberson Greg Lamberson (glamberson) force-pushed the fix/rdpsnd-native-typed-error branch from 60e5d79 to 8282d17 Compare May 18, 2026 14:58
Greg Lamberson (glamberson) added a commit to lamco-admin/IronRDP that referenced this pull request May 20, 2026
Reshape the public surface introduced earlier in this PR before it
lands, to avoid a future breaking touch when ironrdp-server's
anyhow-removal pass arrives. Mirrors the post-Devolutions#1264 hand-rolled
error pattern (Display + core::error::Error impls, no thiserror).

Trait signature was `Result<bool>` (i.e. anyhow::Result through the
file-level `use anyhow::Result`). Two issues:

- New public surface depending on anyhow at a moment when the
  workspace is moving the other way (Devolutions#1277 just removed anyhow from
  rdpsnd-native).
- `Ok(true)` vs `Ok(false)` is stringly typed at the call site: a
  bare bool with no semantic clue.

New shape:

- `CredentialDecision::{Accept, Reject}` for the binary outcome.
- `CredentialValidationError` wrapping any `core::error::Error +
  Send + Sync` for the case where the validator backend itself
  failed (LDAP unreachable, PAM transport error, etc.). Manual
  Display + Error impls; source chains through.
- `validate(&self, &Credentials) -> Result<CredentialDecision,
  CredentialValidationError>`.

The accept/reject/backend-error trichotomy is now explicit at every
call site. Rejection is no longer an error; backend failure is.
Log + bail strings updated to match the trichotomy.

Also addresses the API consistency note: every other configurable
on `RdpServerBuilder<BuilderDone>` goes through `with_*` on the
builder. Added `with_credential_validator(Option<Arc<dyn ...>>)`
following the same shape as `with_connection_handler`. The
post-construction `set_credential_validator` setter is kept (now
takes `Option` to match the field and the builder's setter shape)
for dynamic reconfiguration after `build()`.

Tests added in `server.rs` cover Accept / Reject / backend-error
propagation and `Arc<dyn CredentialValidator>` Send + Sync + 'static
bounds. Integration with `client_accepted` is exercised through
the existing acceptor-side tests once the validator is wired via
the builder; no new integration test in this commit (server-side
precedent per Devolutions#1181 / Devolutions#1187 / Devolutions#1281).

Verification: `cargo test -p ironrdp-server --lib` (14 passed),
`cargo clippy -p ironrdp-server --all-targets -- -D warnings` clean,
`cargo check -p ironrdp-server` clean.

Refs Devolutions#1154, Devolutions#1150, Devolutions#1155.
Greg Lamberson (glamberson) added a commit to lamco-admin/IronRDP that referenced this pull request May 20, 2026
Reshape the public surface introduced earlier in this PR before it
lands, to avoid a future breaking touch when ironrdp-server's
anyhow-removal pass arrives. Mirrors the post-Devolutions#1264 hand-rolled
error pattern (Display + core::error::Error impls, no thiserror).

Trait signature was `Result<bool>` (i.e. anyhow::Result through the
file-level `use anyhow::Result`). Two issues:

- New public surface depending on anyhow at a moment when the
  workspace is moving the other way (Devolutions#1277 just removed anyhow from
  rdpsnd-native).
- `Ok(true)` vs `Ok(false)` is stringly typed at the call site: a
  bare bool with no semantic clue.

New shape:

- `CredentialDecision::{Accept, Reject}` for the binary outcome.
- `CredentialValidationError` wrapping any `core::error::Error +
  Send + Sync` for the case where the validator backend itself
  failed (LDAP unreachable, PAM transport error, etc.). Manual
  Display + Error impls; source chains through.
- `validate(&self, &Credentials) -> Result<CredentialDecision,
  CredentialValidationError>`.

The accept/reject/backend-error trichotomy is now explicit at every
call site. Rejection is no longer an error; backend failure is.
Log + bail strings updated to match the trichotomy.

Also addresses the API consistency note: every other configurable
on `RdpServerBuilder<BuilderDone>` goes through `with_*` on the
builder. Added `with_credential_validator(Option<Arc<dyn ...>>)`
following the same shape as `with_connection_handler`. The
post-construction `set_credential_validator` setter is kept (now
takes `Option` to match the field and the builder's setter shape)
for dynamic reconfiguration after `build()`.

Tests for the trait, the `CredentialDecision` enum, the error
wrapper's source chaining, and the `Send + Sync + 'static` bounds
through `Arc<dyn _>` live in
`crates/ironrdp-testsuite-core/tests/server/credential_validator.rs`,
matching the canonical IronRDP split: public-API tests in
`ironrdp-testsuite-core`, inline `#[cfg(test)]` only for
crate-internal behavior (per the `autodetect.rs` precedent which
has both: inline tests on internal state machine, public-API tests
in `testsuite-core/tests/server/autodetect.rs`).

Verification: `cargo xtask check fmt/lints/tests/typos/locks` all
pass; new tests pass in the testsuite-core harness (4 added).

Refs Devolutions#1154, Devolutions#1150, Devolutions#1155.
Greg Lamberson (glamberson) added a commit to lamco-admin/IronRDP that referenced this pull request May 20, 2026
Reshape the public surface introduced earlier in this PR before it
lands, to avoid a future breaking touch when ironrdp-server's
anyhow-removal pass arrives. Mirrors the post-Devolutions#1264 hand-rolled
error pattern (Display + core::error::Error impls, no thiserror).

Trait signature was `Result<bool>` (i.e. anyhow::Result through the
file-level `use anyhow::Result`). Two issues:

- New public surface depending on anyhow at a moment when the
  workspace is moving the other way (Devolutions#1277 just removed anyhow from
  rdpsnd-native).
- `Ok(true)` vs `Ok(false)` is stringly typed at the call site: a
  bare bool with no semantic clue.

New shape:

- `CredentialDecision::{Accept, Reject}` for the binary outcome.
- `CredentialValidationError` wrapping any `core::error::Error +
  Send + Sync` for the case where the validator backend itself
  failed (LDAP unreachable, PAM transport error, etc.). Manual
  Display + Error impls; source chains through.
- `validate(&self, &Credentials) -> Result<CredentialDecision,
  CredentialValidationError>`.

The accept/reject/backend-error trichotomy is now explicit at every
call site. Rejection is no longer an error; backend failure is.
Log + bail strings updated to match the trichotomy.

Also addresses the API consistency note: every other configurable
on `RdpServerBuilder<BuilderDone>` goes through `with_*` on the
builder. Added `with_credential_validator(Option<Arc<dyn ...>>)`
following the same shape as `with_connection_handler`. The
post-construction `set_credential_validator` setter is kept (now
takes `Option` to match the field and the builder's setter shape)
for dynamic reconfiguration after `build()`.

Tests for the trait, the `CredentialDecision` enum, the error
wrapper's source chaining, and the `Send + Sync + 'static` bounds
through `Arc<dyn _>` live in
`crates/ironrdp-testsuite-core/tests/server/credential_validator.rs`,
matching the canonical IronRDP split: public-API tests in
`ironrdp-testsuite-core`, inline `#[cfg(test)]` only for
crate-internal behavior (per the `autodetect.rs` precedent which
has both: inline tests on internal state machine, public-API tests
in `testsuite-core/tests/server/autodetect.rs`).

Verification: `cargo xtask check fmt/lints/tests/typos/locks` all
pass; new tests pass in the testsuite-core harness (4 added).

Refs Devolutions#1154, Devolutions#1150, Devolutions#1155.
Brings ironrdp-rdpsnd-native into line with the workspace convention
from ARCHITECTURE.md that libraries provide concrete error types
rather than anyhow::Result.

Defines RdpsndNativeErrorKind with four variants covering the failure
modes in DecodeStream::new (UnsupportedFormat, OpusInit, AudioDevice,
StreamBuild). Display and core::error::Error impls are hand-rolled per
the post-Devolutions#1264 pattern in ironrdp-pdu.

DecodeStream::new now returns RdpsndNativeResult<Self> instead of
anyhow::Result<Self>. Foreign error sources (opus2::Error,
cpal::DefaultStreamConfigError, cpal::BuildStreamError) are attached
via Error::with_source so the cause chain remains reachable through
core::error::Error::source().

anyhow moves from [dependencies] to [dev-dependencies] for the cpal
example binary. The internal log of construction failures switches
from "{e:#}" to e.report() to preserve cause-chain output now that
anyhow's alternate-format chain printing is no longer involved.

Signed-off-by: Greg Lamberson <greg@lamco.io>
@glamberson Greg Lamberson (glamberson) force-pushed the fix/rdpsnd-native-typed-error branch from 8282d17 to 4365a85 Compare May 21, 2026 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants