test(rdpeusb): add client/server I/O sequence coverage by uchouT · Pull Request #1406 · Devolutions/IronRDP

uchouT (uchouT) · 2026-07-03T15:41:05Z

add API-boundary tests for server capability exchange and new-device setup
add connected client/server tests for device text, IOCTL, internal IOCTL, cancellation, and transfer IN/OUT flows
cover transfer completions with and without data, including no-ack isochronous OUT transfers
make query_device_text synchronous so every valid query produces a QUERY_DEVICE_TEXT_RSP, as required by MS-RDPEUSB

Copilot

Pull request overview

This PR expands RDPEUSB test coverage to validate client/server setup and end-to-end I/O flows, and aligns the client backend API with MS-RDPEUSB by making query_device_text always produce a QUERY_DEVICE_TEXT_RSP.

Changes:

Added server-side sequence tests for capability exchange and new-device setup.
Added connected client/server round-trip tests for device text, IOCTL/internal IOCTL, cancellation, and transfer IN/OUT (including no-ack OUT).
Updated UrbdrcDeviceBackend::query_device_text to return DeviceText (non-optional), so valid queries always generate a response.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
crates/ironrdp-testsuite-core/tests/rdpeusb/server.rs	New server sequence tests for capability exchange and device setup.
crates/ironrdp-testsuite-core/tests/rdpeusb/mod.rs	Shared rdpeusb test helpers + module wiring for new test modules.
crates/ironrdp-testsuite-core/tests/rdpeusb/io/mod.rs	Connected client/server harness and backend event plumbing for I/O tests.
crates/ironrdp-testsuite-core/tests/rdpeusb/io/requests.rs	New tests for query-device-text, IOCTL/internal IOCTL, and cancel behavior.
crates/ironrdp-testsuite-core/tests/rdpeusb/io/transfers.rs	New tests for transfer IN/OUT completions and no-ack OUT behavior.
crates/ironrdp-testsuite-core/tests/rdpeusb/client.rs	Updated test backend to match the new synchronous device-text query API.
crates/ironrdp-rdpeusb/src/client.rs	Public API change: `query_device_text` now returns `DeviceText` and client always emits `QUERY_DEVICE_TEXT_RSP`.

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

    /// of the request to the server via QUERY_DEVICE_TEXT_RSP message and the RequestId field in
    /// the message MUST match the RequestId in the QUERY_DEVICE_TEXT message.
    ///
    /// [3.3.5.3.5]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpeusb/834f56cc-cfed-4649-8952-0b6486638c28
-    fn query_device_text(&mut self, channel_id: u32, text_type: u32, locale_id: u32) -> PduResult<Option<DeviceText>>;
+    fn query_device_text(&mut self, channel_id: u32, text_type: u32, locale_id: u32) -> PduResult<DeviceText>;


Device text is immutable during a USB lifecycle, therefore making this trait method always respond to device text queries can enforce the trait implementors to do the queries and cache them.

Signed-off-by: uchouT <i@uchout.moe>

Marc-Andre Lureau (elmarco) · 2026-07-08T08:50:56Z

lgtm

Benoît Cortier (CBenoit)

Thank you!

The changes are looking good to me, but the changes to ironrdp-rdpeusb should really be a separate concern from the tests. That said, we don’t publish this crate yet, so I’m good with merging immediately for velocity!

I also have a few other comments below, to be followed upon in separate PRs.

Benoît Cortier (CBenoit) · 2026-07-13T08:21:55Z

    /// [Processing a Query Device Text Message][3.3.5.3.5]:
    ///
    /// After receiving the QUERY_DEVICE_TEXT message, the client forwards the request to the
    /// physical device. When the physical device completes the request, the client sends the result
    /// of the request to the server via QUERY_DEVICE_TEXT_RSP message and the RequestId field in
    /// the message MUST match the RequestId in the QUERY_DEVICE_TEXT message.
    ///
    /// [3.3.5.3.5]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpeusb/834f56cc-cfed-4649-8952-0b6486638c28
-    fn query_device_text(&mut self, channel_id: u32, text_type: u32, locale_id: u32) -> PduResult<Option<DeviceText>>;
+    fn query_device_text(&mut self, channel_id: u32, text_type: u32, locale_id: u32) -> PduResult<DeviceText>;



issue: I think the trait doc now contradicts the synchronous contract of the new signature. The new design contract: "device text is immutable, so implementors must cache it and resolve synchronously", only exists in the commit message, which future implementors won't read. Update the doc paragraph accordingly.

Benoît Cortier (CBenoit) · 2026-07-13T08:24:57Z

+// Refs: [Query Device Text][2.2.6.5] and [Query Device Text Response][2.2.6.6].
+// [2.2.6.5]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpeusb/d03a7696-2d56-4f20-b7a9-a5e72a045956
+// [2.2.6.6]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpeusb/acffdcfa-c792-40a4-a8ee-c545ea5b0a38
+#[test]
+fn query_device_text_round_trip() {
+    let mut device = ConnectedDevice::new();
+
+    let request = device
+        .server
+        .query_device_text(1, 0x0409)
+        .expect("query device text should succeed");
+    let response = only_message(device.send_to_client(request));
+
+    let ClientEvent::QueryDeviceText {
+        channel_id,
+        text_type,
+        locale_id,
+    } = device.next_client_event()
+    else {
+        panic!("expected query device text event");
+    };
+    assert_eq!(channel_id, CHANNEL_ID);
+    assert_eq!(text_type, 1);
+    assert_eq!(locale_id, 0x0409);
+
+    assert!(device.send_to_server(response).is_empty());
+    let ServerEvent::DeviceText(device_text) = device.next_server_event() else {
+        panic!("expected device text event");
+    };
+    assert_eq!(device_text.hresult, DEVICE_TEXT_HRESULT);
+    assert_eq!(device_text.description, DEVICE_TEXT_DESCRIPTION);
+}


suggestion: The round-trip test covers only the success path, not the path the fix in ironrdp-rdpeusb enables: a device with no text -> empty description + a non-zero (error) hresult. That path also exercises a distinct wire encoding: Cch32String is PrefixedString<CchU32, NullCounted> where an empty string encodes as cch=1 (null only) and cch=0 is rejected on decode. Use rstest to add a second case: description: "", error hresult.

Relatedly, the renamed NoopDeviceClientBackend now returns hresult: 0 + empty string, i.e. it models "success with empty text" rather than "no text available". Not sure about this.

Benoît Cortier (CBenoit) · 2026-07-13T08:28:14Z

    /// of the request to the server via QUERY_DEVICE_TEXT_RSP message and the RequestId field in
    /// the message MUST match the RequestId in the QUERY_DEVICE_TEXT message.


note: Pre-existing doc inaccuracy: QueryDeviceText/QueryDeviceTextRsp carry no RequestId; they key on MessageId. This predates the PR so it's out of scope, but worth following up.

Copilot AI review requested due to automatic review settings July 3, 2026 15:41

Copilot started reviewing on behalf of uchouT (uchouT) July 3, 2026 15:41 View session

Copilot AI reviewed Jul 3, 2026

View reviewed changes

uchouT (uchouT) force-pushed the test/urbdrc branch 2 times, most recently from 7ca9723 to 6b48ce4 Compare July 4, 2026 02:20

uchouT (uchouT) added 2 commits July 7, 2026 02:29

fix: always respond to device text queries

8ca43e8

Device text is immutable during a USB lifecycle, therefore making this trait method always respond to device text queries can enforce the trait implementors to do the queries and cache them.

add test for io sequence

b52abbc

Signed-off-by: uchouT <i@uchout.moe>

uchouT (uchouT) force-pushed the test/urbdrc branch from 6b48ce4 to b52abbc Compare July 6, 2026 18:29

Benoît Cortier (CBenoit) approved these changes Jul 13, 2026

View reviewed changes

Benoît Cortier (CBenoit) merged commit 76ad145 into Devolutions:master Jul 13, 2026
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test(rdpeusb): add client/server I/O sequence coverage#1406

test(rdpeusb): add client/server I/O sequence coverage#1406
Benoît Cortier (CBenoit) merged 2 commits into
Devolutions:masterfrom
uchouT:test/urbdrc

uchouT (uchouT) commented Jul 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Marc-Andre Lureau (elmarco) commented Jul 8, 2026

Uh oh!

Benoît Cortier (CBenoit) left a comment

Uh oh!

Benoît Cortier (CBenoit) Jul 13, 2026

Uh oh!

Benoît Cortier (CBenoit) Jul 13, 2026

Uh oh!

Benoît Cortier (CBenoit) Jul 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

		/// of the request to the server via QUERY_DEVICE_TEXT_RSP message and the RequestId field in
		/// the message MUST match the RequestId in the QUERY_DEVICE_TEXT message.

Uh oh!

Conversation

uchouT (uchouT) commented Jul 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Marc-Andre Lureau (elmarco) commented Jul 8, 2026

Uh oh!

Benoît Cortier (CBenoit) left a comment

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants