Devolutions / Devolutions/IronRDP

Audit pass: convert internal-bug *ErrorKind variants to debug_assert!/panic!

Open
#1,274 0 comments 1 reaction 0 assignees View on GitHub
Dominant language
Rust
Stars
3.2k
Forks
275
Avg merge
1d 11h
Merged PRs (30d)
189

Description

## Background

In review of #1267, @CBenoit pushed back on encoding bug-shaped error conditions as `*ErrorKind` variants:

> Bug-shaped conditions, internal invariant violations, "this can't happen,"
> miscalculated buffer sizes on the encode path, should be `debug_assert!` /
> `assert!` / `panic!`, not `Err` variants. The rationale is:
> - The fuzzer detects panics natively. No oracle classification is needed:
> an `Err` is an expected rejection, a SIGABRT is a finding.
> - `debug_assert!` costs nothing in release and turns every test + fuzz
> iteration into a free invariant verifier.
> - Returning `Err` for a bug is the worst-of-both-worlds middle: no fuzzer
> signal, no clean crash, and every caller now has to handle an error that,
> by definition, shouldn't happen.

Process resilience for long-running servers is better solved by a `catch_unwind` boundary at per-connection scope than by typed error categories.

This issue tracks the audit-pass to bring the codebase into alignment with the principle.

## Scope

Walk every `*ErrorKind` variant across the workspace and classify producer call sites:

- **Internal-bug-shaped** (the variant means "we, ourselves, hit an internal contradiction"): convert the producing call site to `debug_assert!` for invariants that should hold but are cheap to verify, or `assert!`/`panic!` for invariants that must hold in release. Variant becomes dead.
- **Wire-protocol or input-shape**: keep as `Err`. The peer can drive these by sending malformed input; the decoder catching it is the correct outcome.
- **API boundary input guard**: keep as `Err` where a typed boundary serves callers that can recover. These are deliberately narrow.

Once producers are gone, the dead variants get removed in a follow-up PR per crate.

## Per-crate sweep

Suggested order, largest variant surface first:

1. `ironrdp-pdu` — the bulk of `DecodeErrorKind` / `EncodeErrorKind` variants are here.
2. `ironrdp-core` — primitive cursor / encode / decode error variants.
3. `ironrdp-connector` — `ConnectorErrorKind` mixes wire-protocol (keep) with internal-state (audit).
4. `ironrdp-server` — `ServerErrorKind` (introduced in #1242). `ServerErrorKind::General` and `::Custom` are catch-all shapes; both will need a closer look once #1242's series lands.
5. Channel crates (`ironrdp-cliprdr`, `ironrdp-rdpdr`, `ironrdp-rdpsnd`, `ironrdp-egfx`, etc.) as they come up.

Each crate is a separate PR.

## Out of scope here, tracked separately

- `catch_unwind` boundary at per-connection scope in `ironrdp-server`. Filed as a separate server-side follow-up issue.
- Production triage tooling (logging, metrics) consuming the new structure. Not blocked on this work since `match err.kind()` continues to work.

## Supersedes

Supersedes #1267 (closed). Replaces the `ErrorClassification` trait approach with the audit-pass direction.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.