Skip to content

fix(sequence)!: require an arrival time on input PDUs - #1757

Draft
Benoît Cortier (CBenoit) wants to merge 3 commits into
cbenoit-extract-ironrdp-sequence-cratefrom
cbenoit-require-frame-timestamps
Draft

fix(sequence)!: require an arrival time on input PDUs#1757
Benoît Cortier (CBenoit) wants to merge 3 commits into
cbenoit-extract-ironrdp-sequence-cratefrom
cbenoit-require-frame-timestamps

Conversation

@CBenoit

@CBenoit Benoît Cortier (CBenoit) commented Aug 21, 2026

Copy link
Copy Markdown
Member

Sequence::step took Option<MonotonicInstant>, so a caller could hand over a PDU without saying when it arrived. The .NET bindings did exactly that: the FFI read its own clock on entry to step, timing how long the managed caller took to get around to the call rather than when the bytes came off the socket.

Sequence now has a single required transition, step_input, taking a StepInput that is either a PDU with the instant its read completed or no PDU at all. step and step_no_input are provided methods on top of it, so a clockless input frame is unrepresentable while each state machine keeps one implementation and the trait stays object safe.

Framed records the instant of the read that filled its buffer, so PDUs drained out of a single read all report that read even when the caller gets to them later. into_inner hands back a Leftover pairing the buffered bytes with their arrival time rather than a bare buffer, so rebuilding a Framed across a TLS upgrade, a websocket handover, or an RDCleanPath response keeps both. Connection.SecurityUpgrade in .NET now reports leftover plaintext instead of silently dropping it.

The FFI exposes MonotonicInstant.Now(), backed by one process-wide clock and with no way to build a reading from a caller-supplied number. The managed Framed calls it immediately after Stream.ReadAsync returns and passes the result down to Sequence.Step.

BREAKING CHANGE: Sequence implementors write step_input instead of step, and Sequence::step requires a MonotonicInstant. Framed::into_inner and Framed::read_by_hint return the arrival time alongside the bytes, Framed::get_inner_mut no longer hands out the buffer, and Framed::last_read_at is gone. In .NET, ISequence.Step, Framed.GetInner, Framed.ReadExact, and Framed.ReadByHint change the same way.

`Sequence::step` took `Option<MonotonicInstant>`, so a caller could hand
over a PDU without saying when it arrived. The .NET bindings did exactly
that: the FFI read its own clock on entry to `step`, timing how long the
managed caller took to get around to the call rather than when the bytes
came off the socket.

`Sequence` now has a single required transition, `step_input`, taking a
`StepInput` that is either a PDU with the instant its read completed or
no PDU at all. `step` and `step_no_input` are provided methods on top of
it, so a clockless input frame is unrepresentable while each state
machine keeps one implementation and the trait stays object safe.

`Framed` records the instant of the read that filled its buffer, so PDUs
drained out of a single read all report that read even when the caller
gets to them later. `into_inner` hands back a `Leftover` pairing the
buffered bytes with their arrival time rather than a bare buffer, so
rebuilding a `Framed` across a TLS upgrade, a websocket handover, or an
RDCleanPath response keeps both. `Connection.SecurityUpgrade` in .NET
now reports leftover plaintext instead of silently dropping it.

The FFI exposes `MonotonicInstant.Now()`, backed by one process-wide
clock and with no way to build a reading from a caller-supplied number.
The managed `Framed` calls it immediately after `Stream.ReadAsync`
returns and passes the result down to `Sequence.Step`.

BREAKING CHANGE: `Sequence` implementors write `step_input` instead of
`step`, and `Sequence::step` requires a `MonotonicInstant`.
`Framed::into_inner` and `Framed::read_by_hint` return the arrival time
alongside the bytes, `Framed::get_inner_mut` no longer hands out the
buffer, and `Framed::last_read_at` is gone. In .NET, `ISequence.Step`,
`Framed.GetInner`, `Framed.ReadExact`, and `Framed.ReadByHint` change
the same way.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@CBenoit
Benoît Cortier (CBenoit) marked this pull request as draft August 21, 2026 15:42
@github-actions github-actions Bot added maintainer-required Maintainer review or intervention is required risk/unknown Risk could not be determined automatically; needs maintainer-level scrutiny scope/core Touches the core architectural tier scope/ffi Affects native or .NET bindings scope/tooling Build, CI, release, or developer tooling scope/web Affects the web/WASM ecosystem size/XXL Size: 1300 or more counted lines or 50 or more files labels Aug 21, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This pull request is size/XXL, 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/XXL threshold.

Represent managed frame timestamps as readonly values and pass their
millisecond reading over the generated ABI. This removes one Rust
allocation and one finalizable managed wrapper from every socket read
while preserving the original read timestamp at each Step boundary.

Expose leftover bytes read-only and reject zero-size hints before I/O in
both Rust drivers and managed framing. Regenerate bindings, drop the
unsupported test asset, and restore unrelated CredSSP generated output.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep Diplomat-generated Step methods on their raw ulong ABI and bridge the typed managed ISequence contract from handwritten partial classes.

Restore all generator-owned output so regeneration remains clean.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintainer-required Maintainer review or intervention is required risk/unknown Risk could not be determined automatically; needs maintainer-level scrutiny scope/core Touches the core architectural tier scope/ffi Affects native or .NET bindings scope/tooling Build, CI, release, or developer tooling scope/web Affects the web/WASM ecosystem size/XXL Size: 1300 or more counted lines or 50 or more files

Development

Successfully merging this pull request may close these issues.

1 participant