fix(connector)!: count the auto-detect header, and only answer connect-time by glamberson · Pull Request #1559 · Devolutions/IronRDP

Greg Lamberson (glamberson) · 2026-08-05T20:07:45Z

Summary

The Network Characteristics Byte Count store was incremented by the payload length alone, so every counted Bandwidth Measure message was reported 8 bytes short.
[MS-RDPBCGR] 3.2.5.14 says to increment it "by the value specified in the payloadLength field plus the size of the header fields (8 bytes)", on each Bandwidth Measure Payload and on the 0x002B Stop. 2.2.14.1.3 pins that 8 by requiring headerLength to be 0x08.
The Start and Stop arms now check the request type rather than matching every variant.

Those two increments are the whole of the connect-time byte count

Worth stating, because 3.2.5.14 also contains a rule that counts every byte received while a window is open, and it's easy to assume it applies here.

It doesn't. That rule belongs to the 0x0014 and 0x0114 Starts, which are the reliable and lossy UDP variants. Connect-time is 0x1014, and its step list says only to clear the stores and start the timer. So on this path the Payload and Stop increments are the entire count, and no framing-layer accumulation is needed.

Why the request-type check is required, not tidying

2.2.14.1.4 sets headerLength to 0x08 for the 0x002B Stop and 0x06 otherwise. A fixed 8-byte addend is therefore only correct once the type is known, which makes the check part of the byte-count fix rather than separate from it.

It has two further effects that are worth having. A continuous-detection Start no longer opens a connect-time window, and a continuous Stop is no longer answered with a connect-time result on the main channel when 3.2.5.14 routes those to a multitransport channel under a different procedure. That procedure also requires a sequence-number correlation on the 0x0629 Stop which this phase does not track, so ignoring is the conservative response.

This overlaps the automated reviewer's fifth finding on #1530, which suggested exactly this narrowing. I'd offered there to fold it into that PR or take it separately; the conditional header size settled it, since the byte count is wrong without it.

This is deliberately not FreeRDP's arithmetic

FreeRDP counts the whole PDU length at the framing layer, bandwidthMeasureByteCount += length in libfreerdp/core/rdp.c after rdp_read_header, for any window including connect-time, and then adds payloadLength again in libfreerdp/core/autodetect.c for the Payload and the Stop. On the connect-time path that counts the payload twice and adds framing bytes the spec doesn't ask for.

I went with the spec rather than the reference here. The reported figure is an informational QoS hint and the server proceeds on receipt either way, so there's no interop cost to being correct. The divergence is called out in a comment at the helper so nobody later "fixes" it toward FreeRDP.

Depends on #1530

Stacked. #1530 introduces the bandwidth window state (connect_time_bw_started_at, connect_time_bw_bytes) that this counts into; on master there's nothing to accumulate. The branch carries #1530's commits, so it builds and tests on its own, and the merge order is #1530 then this.

Because it carries them, the diff shown here is cumulative. This pull request's own change is 3 files, +180/-21: crates/ironrdp-connector/src/connection.rs, crates/ironrdp-connector/src/lib.rs and one connector test. The size label is computed on the cumulative diff, so it counts #1530's four commits too.

Raised by the automated reviewer on #1530, where the prescribed fix is the arithmetic used here.

The reported byte count is measurement-gated

Worth stating for consumers, because it is a deliberate deviation. 3.2.5.14 states the Bandwidth Measure Payload increment unconditionally. Here the Payload bytes are accumulated only while a window is open, which means a driver that reports no arrival times never counts them and its Results carry the Stop's own payload alone against the untimed floor.

A full count paired with a timeDelta the client never measured yields a bandwidth figure that grows with whatever the server chose to send, so this under-reports rather than over-reports. The practical consequence is that a server acting on 3.3.5.14 will pick conservative bandwidth-dependent settings for such a client. Of the drivers in tree only the FFI connector is in that position.

Breaking changes

Two, both in ironrdp-connector, and cargo semver-checks -p ironrdp-connector --baseline-rev master reports exactly these two and nothing else:

Sequence::step takes a third parameter, received_at: Option<MonotonicInstant>. This breaks every external implementor and every caller of the trait. step_no_input keeps its signature and passes None.
ClientConnector gains two private fields, connect_time_bw_started_at and connect_time_bw_bytes, so the struct is no longer constructible by literal downstream.

The clock stays in the driver because a sequence must not read one, which is what keeps the sans-I/O invariant intact. A defaulted trait method would avoid the signature break but would let an implementor override only the old entry point and silently never receive an arrival time, so the required parameter is the safer of the two.

Validation

cargo xtask check fmt/lints/tests/typos/locks all pass on the pinned toolchain, plus cargo xtask wasm check, with fuzz/ built before the lock check.
Four existing expectations move by exactly 8 per counted message: 3584 to 3608 (two Payloads and a Stop), 512 to 520 (one Stop), and two at 1536 to 1552 (a Payload and a Stop each), the second of which is the window-reset test feat(connector)!: pass frame arrival time into Sequence::step #1530 added after this branch was first filed. A fifth, 4096 to 4104, went with the redundant test feat(connector)!: pass frame arrival time into Sequence::step #1530 dropped in review.
Two tests added for the guards: continuous Start, Payload and Stop produce no reply and leave the phase running; and a connect-time Stop arriving after a continuous Start reports the untimed floor against its own payload alone, since the continuous Start opened no window.
Eleven connect-time auto-detect tests pass.

Greg Lamberson (glamberson) · 2026-08-07T15:09:56Z

Answering the size note.

The label is computed on the cumulative diff. This pull request's own change is
2 files, +149/-15: crates/ironrdp-connector/src/connection.rs and one
connector test. The rest is #1530's three commits, which this branch carries
because it is stacked on that pull request.

The note suggests stacked pull requests and adds that they require every branch
to live in this repository. This branch is on a fork, so they cannot, and the
cumulative diff is the consequence. The body states the dependency and the merge
order under "Depends on #1530".

Rebased onto b7657bc, six gates green.

github-actions

Well-motivated and lands correctly. The measurement matches MS-RDPBCGR 3.2.5.14 on the accumulation rule (payloadLength + 8, verified exact against the decoder), the Start/Stop request-type split, the echoed sequence number, and the 0x0003 response type on the main channel. Keeping the clock in the I/O driver rather than in the sans-I/O sequence is the right call and is what forces the trait change. State-machine test coverage is good, and web-time was already in the lockfile, so no new third-party code enters the graph. Nothing blocking. Five items: a comment claiming the wasm32 driver has no clock, which this PR's own web-time dependency contradicts; the gating of Payload accumulation on a timed window, worth surfacing in user-facing docs; an imprecise 0x0429 justification; a note that the API changes are semver-breaking; and a question about whether the FFI path stays clockless. The blocking Framed gained the same stamping with no test.

Protocol analysis: partially_accepted — All ten mappings verified and kept. The decoder reads exactly payloadLength bytes for BW_PAYLOAD and the 0x002B Stop (ironrdp-pdu/src/rdp/autodetect.rs:465-496), so counted_len's payload.len() + 8 is exact, resolving the second uncertainty; test arithmetic also checks out (2048+1024+512 plus 8 per message = 3608). Both "conflicts" items are real deviations but downgraded to non-blocking: 3.2.5.14 is SHOULD-level, the direction is conservative, and neither regresses the base, which already hard-coded timeDelta 1 and counted only the Stop payload. I upgraded the first uncertainty to confirmed: ironrdp-web reaches ironrdp_async::Framed via ironrdp-futures, so wasm32 does observe arrival times. Nothing rejected.

non_blocking / medium — crates/ironrdp-connector/src/connection.rs:721-724
The Stop-arm comment states the driver "reports no arrival times at all, as the wasm32 and FFI drivers do for the whole connection". This is wrong for wasm32 and is contradicted inside this same PR: ironrdp-web drives ironrdp_futures::LocalFuturesFramed, which is a re-export of ironrdp_async::Framed (crates/ironrdp-futures/src/lib.rs:5,83), and crates/ironrdp-async/src/framed.rs:308-320 adds the web-time dependency specifically so that build stamps last_read_at via Performance.now(). Only the FFI path is clockless. The same incorrect claim is repeated in the new test doc for connect_time_bandwidth_without_a_clock_reports_the_stop_payload_alone. A maintainer trusting this comment could conclude web-time buys nothing and remove the dependency added here, silently regressing the browser build to the 1 ms floor. The protocol handoff raised this only as an uncertainty; it is confirmed.
non_blocking / medium — crates/ironrdp-connector/src/connection.rs:702-708
Bandwidth Measure Payload bytes are accumulated only while connect_time_bw_started_at is Some. MS-RDPBCGR 3.2.5.14 states the Payload increment unconditionally, with no precondition that a window was successfully timed. The deviation is deliberate and reasoned in-comment, is SHOULD-level, and under-reports rather than over-reports, so it does not warrant rejection; it also is not a regression, since the prior code reported only the Stop payload. The practical cost is that on any clockless driver the whole feature is inert: byteCount collapses to 520 for the exercised sizes, which is what the two new tests pin. Worth stating in the crate docs or a CHANGELOG note that the reported byteCount is measurement-gated, since a server acting on 3.3.5.14 will pick conservative bandwidth-dependent settings for those clients.
non_blocking / low — crates/ironrdp-connector/src/connection.rs:738-744
The comment justifies restricting the reply to 0x002B by saying 0x0429 and 0x0629 are "answered on a multitransport channel". MS-RDPBCGR 3.2.5.14 in fact provides that a 0x0429 Stop received in an Auto-Detect Request PDU is answered on the main RDP channel with responseType 0x000B; what actually puts it out of scope here is that 2.2.14.1.4 scopes that case to after the RDP Connection Sequence completes, which this ConnectTimeAutoDetection state is not. The restriction itself is correct and is an improvement over the base, which answered every Stop with the connect-time responseType; only the stated reason is imprecise. Given how much weight the surrounding comments carry as the design record, the wording is worth correcting.
non_blocking / low — crates/ironrdp-connector/src/lib.rs:367-372
Sequence::step gains a required parameter, which breaks every external implementor and caller of a public trait in ironrdp-connector 0.10.0, and ClientConnector (crates/ironrdp-connector/src/connection.rs:279) gains private fields while remaining a non-non_exhaustive struct whose fields were all pub, so downstream struct-literal construction also breaks. The design itself is sound: keeping the clock in the driver preserves the sans-I/O invariant that a sequence must not read a clock, and a default-method alternative would create a footgun where an implementor overrides only the old entry point. The ask is just that the release metadata and changelog record this as breaking so release-plz produces the right bump rather than a patch release.
question / low — ffi/src/connector/mod.rs:159-159
The FFI connector hard-codes None, so the .NET bindings never open a measurement window and permanently report timeDelta of 1 with only the Stop's own payload counted. Sequence::step's doc says the None case is "why it stays", which reads as a permanent decision. Is the intent that FFI consumers stay on the floor indefinitely, or is a follow-up planned to let the caller pass a millisecond reading through the FFI step signature (the caller owns its read loop and already knows when the read completed)? Answering this determines whether the ffi None is a deliberate terminal state or a gap worth a tracking issue.

Benoît Cortier (CBenoit) · 2026-08-09T15:07:33Z

I think we can partially ignore the result of the automated review here, it ran on stacked code that is already being reviewed separately. Note that draft PRs will not be automatically reviewed until the draft status is lifted.

github-actions · 2026-08-10T10:02:51Z

This pull request is size/XL, so automated review is disabled for it: a change this large is hard to review well in one piece, whether by a human or a model.

Please split it into focused pull requests that can each be reviewed on their own. When the parts build on each other, stacked pull requests let you open each one on top of the last without waiting for the one below to merge. Stacks require every branch to live in this repository, so from a fork, please open separate pull requests instead.

Automated review resumes once the change is below the size/XL threshold.

…ationale Review round on Devolutions#1559. All prose, no behaviour change. The Stop-arm comment and the matching test doc said wasm32 reports no arrival times. It does: ironrdp-web reaches ironrdp_async::Framed through ironrdp-futures, and Framed stamps every read from Performance.now() by way of web-time, which is the reason that dependency exists. Only the FFI connector is clockless. Both texts now name it alone and say why the browser build is not in that position, so the dependency does not read as removable. The 0x002B restriction was justified by saying 0x0429 and 0x0629 are answered on a multitransport channel. That is wrong for 0x0429: 3.2.5.14 gives it two forms and answers the Auto-Detect Request form on the main RDP channel with responseType 0x000B. What scopes it out here is 2.2.14.1.4, which places that form after the RDP Connection Sequence completes. The comment now says that, and picks up the byte-count and responseType splits the old wording blurred. Sequence::step gains a paragraph on the consequence of passing None: the byte count is measurement-gated, so a clockless driver reports the Stop payload alone and a server acting on 3.3.5.14 picks conservative settings. MonotonicInstant's doc no longer implies the FFI case is permanent; the shim has no parameter to receive a reading through, which is not the same as its caller being unable to supply one.

Connect-time bandwidth measurement needs to know when bytes arrived, and nothing in the sans-I/O layer could tell it. The connector answered a Bandwidth Measure Stop with a fabricated interval because the real one was not observable from where the response is built. Introduce MonotonicInstant, a millisecond counter with no epoch and no platform dependency, and make it a required parameter of Sequence::step. The I/O drivers already know when a read completed, so Framed records the arrival time of each read and hands it to the state machine. Callers that genuinely have no frame to time (the no-input path) use step_no_input. With arrival times available, the connector measures for real: a Bandwidth Measure Start opens a window, Payload messages accumulate their byte counts, and Stop reports the elapsed time between its own arrival and the Start's. A Stop with no preceding Start still gets its response, because the server blocks without one, but reports a zero interval rather than inventing a figure. BREAKING CHANGE: Sequence::step takes a MonotonicInstant. Implementors must add the parameter; callers driving a sequence by hand must supply an arrival time or switch to step_no_input.

… was The connect-time Bandwidth Measure handler opened a counting window on every Start and reported the accumulated Payload bytes on the Stop, whatever the elapsed time turned out to be. On a driver that reports no arrival times the elapsed time is always zero and falls back to the 1 ms floor, so the reply paired a full byte count with an interval the client never measured. A server computing byteCount * 8 / timeDelta then scales its estimate with however much payload it chose to send, and the wasm32 and FFI drivers are in that position for the whole connection. Model the absence of a reading as the absence of a reading: Sequence::step takes Option<MonotonicInstant>, and MonotonicInstant::ZERO is gone. It had served as both the clock's origin and the "did not measure" sentinel, which is what made the two cases indistinguishable at the point where it mattered. A window now opens only when the Start carried a reading, so the two outcomes separate. A driver with no clock opens nothing, accumulates nothing, and answers the Stop with its own payload alone. A window that was timed reports every byte per MS-RDPBCGR 3.2.5.14, including when the elapsed time rounds down to the floor because one socket read delivered the whole exchange: the bytes did arrive inside that millisecond, so the floor bounds a real measurement. Framed::last_read_at becomes Option<MonotonicInstant> and the wasm arm of monotonic_now returns None rather than a fixed instant.

Two gaps the deep reviewer named on Devolutions#1530 that were left open. A second Bandwidth Measure Start has to clear both stores and restart the timer per MS-RDPBCGR 3.2.5.14. That was the one branch the spec explicitly requires and nothing exercised it; the new test fails independently if either half of the reset is dropped. Nothing pinned Framed::last_read_at either. The connector tests feed Sequence::step hand-picked instants, so a regression that stopped stamping reads would leave them green while every measured window collapsed to the unmeasurable floor. These cover both directions: a stamp that stops advancing, and a stamp taken on drain rather than on the read, which would break the rule that a PDU served from the buffer keeps the arrival time of the read that filled it. They live in ironrdp-testsuite-core rather than inline because ironrdp-async sets [lib] test = false, so an inline module would compile and never run.

`web-time` is `std::time` on every target except `wasm32-unknown-unknown`, where it reads `Performance.now()`, so the driver clock no longer has a hole in the browser build and `monotonic_now` is one function again rather than two behind a `cfg`. The crate is already in `Cargo.lock` through quinn and rustls-pki-types, so this is an edge and not a new dependency. The `Option` stays. `ffi/src/connector/` does not use `Framed`: the embedder owns the read loop and hands `step` bytes it has already read, so a clock read there would stamp when the caller called rather than when the bytes arrived, which is the error this series exists to remove. `MonotonicInstant`'s doc said the absence was about wasm; it now says what it is actually about. Test pass: ten to eight, no assertion lost. `connect_time_bandwidth_reports_the_measured_interval_and_total_bytes` was subsumed by `..._measured_window_reports_every_payload`, which asserts the same interval and also covers accumulation. `nothing_read_yet_means_no_arrival_time` survives as the opening assertion of `each_socket_read_advances_the_arrival_time`.

…-time The Network Characteristics Byte Count store was incremented by the payload length alone. [MS-RDPBCGR] 3.2.5.14 says to increment it "by the value specified in the payloadLength field plus the size of the header fields (8 bytes)", on each Bandwidth Measure Payload and on the 0x002B Stop, so every counted message was reported 8 bytes short. 2.2.14.1.3 pins that 8 by requiring headerLength to be 0x08. Those two increments are the whole of the connect-time byte count. The section's other rule, which counts every byte received while the window is open, belongs to the 0x0014 and 0x0114 Starts. Those are the reliable and lossy UDP variants; connect-time is 0x1014 and its step list has no such clause. The Start and Stop arms now check the request type instead of matching every variant. That is required rather than tidy: 2.2.14.1.4 sets headerLength to 0x08 only for the 0x002B Stop and 0x06 otherwise, so a fixed 8-byte addend is correct only once the type is known. It also stops a continuous-detection request being answered with a connect-time result on the wrong channel, and 3.2.5.14 asks for a sequence-number correlation on the 0x0629 Stop that this phase does not track. This deliberately does not follow FreeRDP, which counts the whole PDU length at the framing layer in rdp.c after rdp_read_header, for any window including connect-time, then adds payloadLength again in autodetect.c. On this path that counts the payload twice and adds framing bytes the spec does not ask for. The figure is an informational QoS hint and the server proceeds either way, so following the spec costs no interop. Four existing expectations move by 8 per counted message: 4096 to 4104, 1536 to 1552, 3584 to 3608, 512 to 520. Two tests added for the request-type guards. Stacked on Devolutions#1530, which introduces the window state this counts into.

…ationale Review round on Devolutions#1559. All prose, no behaviour change. The Stop-arm comment and the matching test doc said wasm32 reports no arrival times. It does: ironrdp-web reaches ironrdp_async::Framed through ironrdp-futures, and Framed stamps every read from Performance.now() by way of web-time, which is the reason that dependency exists. Only the FFI connector is clockless. Both texts now name it alone and say why the browser build is not in that position, so the dependency does not read as removable. The 0x002B restriction was justified by saying 0x0429 and 0x0629 are answered on a multitransport channel. That is wrong for 0x0429: 3.2.5.14 gives it two forms and answers the Auto-Detect Request form on the main RDP channel with responseType 0x000B. What scopes it out here is 2.2.14.1.4, which places that form after the RDP Connection Sequence completes. The comment now says that, and picks up the byte-count and responseType splits the old wording blurred. Sequence::step gains a paragraph on the consequence of passing None: the byte count is measurement-gated, so a clockless driver reports the Stop payload alone and a server acting on 3.3.5.14 picks conservative settings. MonotonicInstant's doc no longer implies the FFI case is permanent; the shim has no parameter to receive a reading through, which is not the same as its caller being unable to supply one.

github-actions Bot added maintainer-required Maintainer review or intervention is required risk/unknown scope/core Touches the core architectural tier scope/ffi Affects native or .NET bindings scope/web Affects the web/WASM ecosystem size/XL Size: 800 or more lines of code labels Aug 5, 2026

Greg Lamberson (glamberson) force-pushed the fix/connect-time-bandwidth-byte-count branch 2 times, most recently from ab66cf5 to e4620b9 Compare August 7, 2026 15:08

Greg Lamberson (glamberson) mentioned this pull request Aug 7, 2026

feat(connector)!: pass frame arrival time into Sequence::step #1530

Open

Greg Lamberson (glamberson) force-pushed the fix/connect-time-bandwidth-byte-count branch from e4620b9 to 16bccde Compare August 7, 2026 16:10

github-actions Bot reviewed Aug 9, 2026

View reviewed changes

Comment thread crates/ironrdp-connector/src/connection.rs Outdated

Comment thread crates/ironrdp-connector/src/connection.rs

Comment thread crates/ironrdp-connector/src/connection.rs Outdated

Comment thread crates/ironrdp-connector/src/lib.rs

Comment thread ffi/src/connector/mod.rs

Greg Lamberson (glamberson) changed the title ~~fix(connector): count the auto-detect header, and only answer connect-time~~ fix(connector)!: count the auto-detect header, and only answer connect-time Aug 10, 2026

Greg Lamberson (glamberson) force-pushed the fix/connect-time-bandwidth-byte-count branch from 16bccde to 2c7ae64 Compare August 10, 2026 10:16

Greg Lamberson (glamberson) added 2 commits August 10, 2026 05:58

Greg Lamberson (glamberson) added 4 commits August 10, 2026 05:58

Greg Lamberson (glamberson) force-pushed the fix/connect-time-bandwidth-byte-count branch from 2c7ae64 to 804f9d1 Compare August 10, 2026 16:18

holovchenko mentioned this pull request Aug 11, 2026

connector: server Auto-Detect Requests sent after the ConnectTimeAutoDetection phase kill the connection (license exchange, then capabilities exchange) #1629

Open

Navigation Menu

Uh oh!

fix(connector)!: count the auto-detect header, and only answer connect-time - #1559

fix(connector)!: count the auto-detect header, and only answer connect-time#1559
Greg Lamberson (glamberson) wants to merge 6 commits into
Devolutions:masterfrom
lamco-admin:fix/connect-time-bandwidth-byte-count

Greg Lamberson (glamberson) commented Aug 5, 2026 •

edited

Loading

Uh oh!

Greg Lamberson (glamberson) commented Aug 7, 2026

Uh oh!

github-actions Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Benoît Cortier (CBenoit) commented Aug 9, 2026

Uh oh!

github-actions Bot commented Aug 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

Greg Lamberson (glamberson) commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Those two increments are the whole of the connect-time byte count

Why the request-type check is required, not tidying

This is deliberately not FreeRDP's arithmetic

Depends on #1530

The reported byte count is measurement-gated

Breaking changes

Validation

Uh oh!

Greg Lamberson (glamberson) commented Aug 7, 2026

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Benoît Cortier (CBenoit) commented Aug 9, 2026

Uh oh!

github-actions Bot commented Aug 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Greg Lamberson (glamberson) commented Aug 5, 2026 •

edited

Loading