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

[Greg Lamberson (glamberson)]

Summary

• Connect-time bandwidth measurement needs to know when bytes arrived, and nothing in the sans-I/O layer could tell it. fix(connector): answer connect-time Bandwidth Measure to unblock FreeRDP servers #1465, now merged, answers the server's Bandwidth Measure Stop with a nominal interval for exactly that reason: the connector has no way to observe the real one.
• Introduce MonotonicInstant, a millisecond counter with an arbitrary epoch, and make Option<MonotonicInstant> 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. A driver with no clock passes None.
• With arrival times available, measure 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.

#1465 has merged, so this applies directly to master and carries no merge-order dependency. That PR was the FreeRDP unblock on its own; this is the design change behind it, split out at Benoît Cortier (@CBenoit)'s suggestion in review.

Why the clock lives in the driver

Two reasons, both of which rule out having the sequence read a clock itself.

std::time::Instant::now panics on wasm32-unknown-unknown, and ironrdp-connector compiles for it. ironrdp-web reaches ironrdp-async through ironrdp-futures, so the clock read is cfg'd there too: the wasm arm returns None rather than panicking on the first socket read.

More fundamentally, a sequence that timestamps its own entry measures how fast it drained an already-filled buffer, not how long the bytes took to arrive. Framed stamps the actual socket read, so a PDU served entirely from buf correctly reports the read that filled it.

Why the parameter is an Option

Three situations look alike at the Stop and are not alike, and collapsing them produces a worse figure than the nominal one #1465 sends.

A Stop with no preceding Start has no window to time. A window whose Start, Payload and Stop arrived in one socket read carries the same instant on all three, so the elapsed time rounds down to zero; TCP coalescing makes this the common case on a fast link, because the connect-time payload is small. And a driver with no clock reports nothing at any point, which is not a measurement of zero but the absence of one.

Only the middle case is a measurement. Making the parameter Option<MonotonicInstant> is what keeps it distinguishable: a window opens only when the Start carried a reading, so a driver with no clock opens none, 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 floors to 1 ms, because the bytes really did arrive inside that millisecond.

Both untimed cases report a 1 ms floor rather than 0, since a server computing byteCount * 8 / timeDelta divides by it.

There is no MonotonicInstant::ZERO. An earlier revision used it as both the clock's origin and the "did not measure" sentinel, which is precisely what made the wasm32 and FFI drivers indistinguishable from a fast link: they would have paired a full byte count with an interval the client never measured, and the reported bandwidth would have grown with however much payload the server chose to send.

Breaking change

Sequence::step takes an Option<MonotonicInstant>. Implementors must add the parameter; callers driving a sequence by hand must supply an arrival time or switch to step_no_input, which passes None for them. All 8 in-tree implementors and the call sites across ironrdp-acceptor, ironrdp-async, ironrdp-blocking, ironrdp-client, ironrdp-web and ffi are updated here. Framed::last_read_at returns Option<MonotonicInstant>.

Stacked on this

#1559 is stacked on this pull request and carries its three commits, so the merge order is this one then #1559. Landing this shrinks #1559's diff to its own change, 2 files and +149/-15 in connection.rs plus one connector test.

#1559 counts the 8-byte auto-detect header into the byte count introduced here, and narrows the Start and Stop arms to the connect-time request type. Both come from the automated reviewer's findings on this pull request.

Why this is one PR

The automation flags this as size/XL and asks for it to be split. The three commits here do not separate into reviewable pull requests.

The first changes the Sequence::step signature, and every implementor and caller moves with it: connector, acceptor, async, blocking, client, web, the FFI and the tests. That is 19 of the 23 files and +474/-116 on its own, and a pull request holding the trait change without them does not build.

The second exists because of the review on the first. It distinguishes a window that was never timed from one measured as zero, which is the fix for the finding that a wasm or FFI client would otherwise report a fabricated bandwidth. Splitting it out means filing the first commit with a defect already raised against it.

The third is the tests for both.

Stacked pull requests are the remedy the note suggests, and it adds that they require every branch to live in this repository. This branch is on a fork, so that path is closed.

Validation

• cargo xtask check fmt/typos/lints/tests/locks all pass on the pinned toolchain, fuzz/ built before the lock check.
• cargo xtask wasm check passes. Worth stating separately: this is the gate that catches the Instant::now hazard above, since the workspace default target never compiles that crate for wasm.
• Six new tests in crates/ironrdp-testsuite-core/tests/connector/autodetect.rs: the interval and byte count match what the driver observed across a Start/Payload/Stop window; several Payload messages accumulate across a timed window; a window delivered in one read floors the interval with every byte still counted; a driver reporting no arrival times answers with the Stop payload alone; a Stop with no preceding Start is answered with the floor; and a second Start clears both stores and restarts the timer, which 3.2.5.14 requires and which nothing else covered.
• Four more in a new crates/ironrdp-testsuite-core/tests/async_framed.rs covering Framed::last_read_at itself, since the tests above feed Sequence::step hand-picked instants and would stay green if the driver stopped stamping reads. They pin that an unread Framed reports no arrival, that each socket read advances the stamp, that PDUs sharing a read share its stamp rather than being stamped when the caller drains them, and that a Framed rebuilt around leftover bytes reports no arrival for them, which is the case ironrdp-tokio's split and unsplit helpers produce. Each was checked by planting the corresponding regression and confirming the test fails.
• Those live in ironrdp-testsuite-core rather than inline because ironrdp-async sets [lib] test = false, so an inline #[cfg(test)] module would compile and never run. This adds ironrdp-async as a dev-dependency of ironrdp-testsuite-core.

[Copilot]

Pull request overview

Introduces driver-observed PDU arrival timestamps for accurate connect-time bandwidth measurement.

Changes:

• Adds MonotonicInstant and updates the breaking Sequence::step API.
• Records read completion times in async and blocking drivers.
• Measures bandwidth windows and adds regression tests.

Reviewed changes

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

Show a summary per file

File	Description
ffi/src/connector/mod.rs	Adapts FFI connector stepping.
ffi/src/connector/activation.rs	Adapts FFI activation stepping.
crates/ironrdp-web/src/session.rs	Supplies an inert browser timestamp.
crates/ironrdp-testsuite-core/tests/session/connection_activation.rs	Updates activation tests.
crates/ironrdp-testsuite-core/tests/server/acceptor.rs	Updates acceptor tests.
crates/ironrdp-testsuite-core/tests/connector/autodetect.rs	Tests bandwidth measurement behavior.
crates/ironrdp-connector/src/license_exchange.rs	Accepts arrival timestamps.
crates/ironrdp-connector/src/lib.rs	Defines the timestamp type and API.
crates/ironrdp-connector/src/connection.rs	Implements bandwidth measurement.
crates/ironrdp-connector/src/connection_finalization.rs	Accepts arrival timestamps.
crates/ironrdp-connector/src/connection_activation.rs	Propagates arrival timestamps.
crates/ironrdp-connector/src/channel_connection.rs	Accepts arrival timestamps.
crates/ironrdp-client/src/rdp.rs	Updates manual sequence stepping.
crates/ironrdp-blocking/src/framed.rs	Records blocking-read timestamps.
crates/ironrdp-blocking/src/connector.rs	Passes blocking-read timestamps.
crates/ironrdp-async/src/framed.rs	Records and forwards async-read timestamps.
crates/ironrdp-acceptor/src/finalization.rs	Accepts arrival timestamps.
crates/ironrdp-acceptor/src/connection.rs	Propagates arrival timestamps.
crates/ironrdp-acceptor/src/channel_connection.rs	Accepts arrival timestamps.

---

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

[github-actions]

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.

[Greg Lamberson (glamberson)]

Answering the size note.

The three commits here do not separate into reviewable pull requests. The first
changes the Sequence::step signature, and every implementor and caller moves
with it: connector, acceptor, async, blocking, client, web, the FFI and the
tests, 19 of the 23 files and +474/-116 by itself. A pull request holding the
trait change without them does not build. The second is the fix for the review
finding on the first, that a wasm or FFI client would report a fabricated
bandwidth when nothing was measured, so extracting it means filing the first
with a known defect. The third is the tests for both.

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 that path is closed.

The body now says this under "Why this is one PR". It also now records that
#1559 is stacked on this one, which was missing: the merge order is this pull
request then #1559, and landing this one shrinks #1559's diff to 2 files.

[Benoît Cortier (CBenoit)]

Thank you for the follow up

[Benoît Cortier (CBenoit)]

I would like to address the comment I added here, so we actually support that for WASM as well.

If we can actually get a MonotonicTime on all platforms, we can maybe consider removing the Option as well?

Also perform a pass of test minimization if you can

[Greg Lamberson (glamberson)]

Both remaining points, since neither has a thread.

Removing the Option: I don't think it can go, and the blocker is the FFI rather
than the browser. ffi/src/connector/ doesn't use Framed. The embedder owns
the socket and the read loop and hands step bytes it has already read, so a
clock read inside the shim would stamp when the caller got around to calling
step, not when the bytes arrived. That is the same error as a sequence reading
its own clock, which is what this PR removes. The alternatives are a new FFI
surface so the embedder can pass the time, or a reading we know is wrong.

What web-time does change is what None means: no longer "wasm has no clock" but
"this driver does not observe arrival times", which is permanent for a
caller-driven loop rather than a gap waiting to be filled. The MonotonicInstant
doc now says that; the Sequence::step parameter doc already did.

Test minimization: 10 down to 8, no assertion lost.
connect_time_bandwidth_reports_the_measured_interval_and_total_bytes is
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.

I kept both floor tests, which look like duplicates and are not: in
coalesced_into_one_read a window is open, measured zero elapsed, and 1536 bytes
are still counted; in without_a_clock no window opened and only the Stop's own
512 are. That distinction is what the second commit exists for.

[Benoît Cortier (CBenoit)]

Removing the Option: I don't think it can go, and the blocker is the FFI rather than the browser. ffi/src/connector/ doesn't use Framed. The embedder owns the socket and the read loop and hands step bytes it has already read, so a clock read inside the shim would stamp when the caller got around to calling step, not when the bytes arrived. That is the same error as a sequence reading its own clock, which is what this PR removes. The alternatives are a new FFI surface so the embedder can pass the time, or a reading we know is wrong.

I think the alternative of extending the FFI surface is the right move. The FFI surface there was already closely modeled after the Rust interface, so it would be natural we extend it the same way: a MonotonicTime wrapper + a way to produce a "now" value, then pass it through the step function. We gain feature parity, as we should, and at this point the question becomes: why bother supporting the alternative path where we don’t know the frame arrival time (None) when it seems possible to have Some in all the consumers. I’m good with requiring the frame arrival monotonic time everywhere and consistently as a new API contract.