Skip to content

perf(connector): add capture-backed replay - #1947

Open
Benoît Cortier (CBenoit) wants to merge 8 commits into
cbenoit-unified-benchmark-entry-pointsfrom
cbenoit-connector-driven-replay
Open

perf(connector): add capture-backed replay#1947
Benoît Cortier (CBenoit) wants to merge 8 commits into
cbenoit-unified-benchmark-entry-pointsfrom
cbenoit-connector-driven-replay

Conversation

@CBenoit

@CBenoit Benoît Cortier (CBenoit) commented Sep 10, 2026

Copy link
Copy Markdown
Member

Drive the no-NLA capture through the production connector and an ActiveStage so connection and session work can be measured independently per capture.

Verify immutable capture counters, protocol output sequencing, and semantic output before timing. The strict contract validates captured Save Session Info events, reconnect-cookie position and logon ID, termination reasons, and the deterministic bitmap update; reconnect credential bytes are never recorded. Strict licensing verification retains the stable PlatformId and normalizes only ClientRandom and EncryptedPreMasterSecret. Timed executions retain fresh-state framing and session validation without hashing output payloads.

Model TLS completion as an external boundary and retain opaque static-channel payload semantics as an explicit exclusion.

Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com

@github-actions github-actions Bot added kind/protocol Affects RDP or related protocol behavior risk/medium Behavioral change that does not substantially alter a core public API scope/tooling Build, CI, release, or developer tooling size/L Size: up to 899 counted lines and 20 files; exceeds M in either measure labels Sep 10, 2026
@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 size/XL Size: up to 1299 counted lines and 49 files; exceeds L in either measure and removed size/L Size: up to 899 counted lines and 20 files; exceeds M in either measure risk/medium Behavioral change that does not substantially alter a core public API labels Sep 10, 2026
@github-actions github-actions Bot added risk/medium Behavioral change that does not substantially alter a core public API and removed risk/unknown Risk could not be determined automatically; needs maintainer-level scrutiny kind/protocol Affects RDP or related protocol behavior maintainer-required Maintainer review or intervention is required labels Sep 10, 2026
@github-actions github-actions Bot added risk/low Self-contained change with no cross-crate behavioral effect and removed risk/medium Behavioral change that does not substantially alter a core public API labels Sep 10, 2026
@CBenoit
Benoît Cortier (CBenoit) force-pushed the cbenoit-connector-driven-replay branch from 53b0037 to 50af859 Compare September 10, 2026 20:56
@CBenoit
Benoît Cortier (CBenoit) force-pushed the cbenoit-connector-driven-replay branch from 50af859 to a8d7c08 Compare September 10, 2026 21:01
@github-actions github-actions Bot added risk/medium Behavioral change that does not substantially alter a core public API and removed risk/low Self-contained change with no cross-crate behavioral effect labels Sep 10, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bench-crate-only addition that drives the production ClientConnector through the pinned no-nla-accepted capture and an ActiveStage, with strict counters, PDU-sequence, and semantic-fingerprint preflight outside the timed path. The design is sound, well documented, and confined to ironrdp-bench; no correctness defect was found. One open question remains: the pinned graphics_updates=0 contract is unreconciled with the same capture's passive corpus entry recording 78 updates, plausibly explained by the documented EGFX/opaque-static-channel exclusions and the synthesized client configuration but not stated. The rest are low-severity maintainability items: duplicated outbound decode walks, prepare(id) hardcoding the capture, duplicated cache-path logic, a redundant capture re-hash/read, a cloned error type, a dead non-TLS branch, a double-enforced constant counter, and the strict contract missing from the documented CI regression entry point.

Comment thread crates/ironrdp-bench/src/connector_replay.rs
Comment thread crates/ironrdp-bench/src/connector_replay.rs
Comment thread crates/ironrdp-bench/src/connector_replay.rs
Comment thread crates/ironrdp-bench/src/connector_replay.rs
Comment thread crates/ironrdp-bench/src/replay.rs
Comment thread crates/ironrdp-bench/src/connector_replay.rs
Comment thread crates/ironrdp-bench/src/connector_replay.rs
Comment thread crates/ironrdp-bench/src/connector_replay.rs
Comment thread crates/ironrdp-bench/src/connector_replay.rs Outdated
@github-actions github-actions Bot added the ai-reviewed/1 One automated review completed label Sep 10, 2026
@CBenoit
Benoît Cortier (CBenoit) force-pushed the cbenoit-connector-driven-replay branch from a8d7c08 to fe78e53 Compare September 11, 2026 01:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The immutable PDU contract rejects the expected license request, and strict verification omits important session outputs.

Review details

Suppressed comments (2)

crates/ironrdp-bench/src/connector_replay.rs:61

  • The contract leaves no slot for the license request that this workload is designed to produce. outbound_frames + active_response_frames is 46, while the 12-entry prefix plus 34 session frames also consumes all 46 entries; inspect_send_data classifies the generated request as ClientNewLicenseRequest, so every verify() and replay() fails sequence validation. Include that PDU after ClientInfo and reduce the remaining session-frame count to 33.
    session_data_frames: 34,

crates/ironrdp-bench/src/connector_replay.rs:875

  • Strict verification silently ignores every semantic ActiveStageOutput except response frames; graphics regions/pixels, termination, deactivation, save-session state, pointer events, and other outputs never affect the fingerprint or contract. Consequently the advertised semantic preflight can pass even if the session terminates or the deterministic bitmap renders incorrect data. In the strict path, fingerprint or explicitly validate every expected output variant (including the exact deterministic region/framebuffer), and reject unsupported state-changing outputs.
    for output in outputs {
        if let ActiveStageOutput::ResponseFrame(frame) = output {
            response_frames += validate_outbound_frames(
  • Files reviewed: 7/7 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@github-actions github-actions Bot added risk/low Self-contained change with no cross-crate behavioral effect size/XXL Size: 1300 or more counted lines or 50 or more files and removed risk/medium Behavioral change that does not substantially alter a core public API size/XL Size: up to 1299 counted lines and 49 files; exceeds L in either measure labels Sep 11, 2026
@CBenoit
Benoît Cortier (CBenoit) force-pushed the cbenoit-connector-driven-replay branch from c208273 to b7be514 Compare September 11, 2026 03:12
@github-actions github-actions Bot added maintainer-required Maintainer review or intervention is required kind/protocol Affects RDP or related protocol behavior risk/medium Behavioral change that does not substantially alter a core public API and removed risk/low Self-contained change with no cross-crate behavioral effect maintainer-required Maintainer review or intervention is required labels Sep 11, 2026
@CBenoit
Benoît Cortier (CBenoit) force-pushed the cbenoit-connector-driven-replay branch from b7be514 to b20fd41 Compare September 11, 2026 04:53

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR 1947 adds a capture-backed connector replay benchmark that drives the production ClientConnector to Connected, builds an ActiveStage, and validates an immutable counter, outbound-PDU, semantic-output, and fingerprint contract outside timing. Verification is fail-closed: passive preflight, TLS-only negotiation checks, CredSSP/multitransport guards, normalized-credential assertions, and license-secret exclusion all check out against connector/session sources. No correctness defects found; the code-compressor's five valid low-severity maintenance candidates (an unreachable empty check, contract/measurement field mirroring, duplicated capture-path resolution, first_frame reimplementing framed_stream, and a twice-decoded outbound chain) are accepted as-is.

Comment thread crates/ironrdp-bench/src/connector_replay.rs Outdated
Comment thread crates/ironrdp-bench/src/connector_replay.rs
Comment thread crates/ironrdp-bench/src/replay.rs
Comment thread crates/ironrdp-bench/src/connector_replay.rs
Comment thread crates/ironrdp-bench/src/connector_replay.rs
@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

This pull request may overlap with #1944.

Both PRs define capture-replay benchmark workloads in ironrdp-bench with strict preflight verification outside Criterion timing, identifiable workload IDs, a standalone single-execution measurement binary, and xtask README guidance, touching the same files: benches/capture_replay.rs, src/bin/capture_replay_bench.rs, src/replay.rs, and xtask/README.md.

This notice is advisory only. Automated review continues as usual, and how these pull requests relate is for maintainers and authors to decide.

Note

LLM-assisted content (no human feedback).

Drive the no-NLA capture through the production connector and an ActiveStage
so connection and session work can be measured independently per capture.

Model TLS completion as an external boundary and retain opaque static-channel
payload semantics as an explicit exclusion.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Validate fixed capture counters and semantic output before timing each connector replay.

Keep SHA-256 work in strict preflight while timed runs retain fresh-state
framing, PDU-sequence, and session checks.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep the stable PlatformId field in strict licensing output verification
while normalizing only newly generated client-random and encrypted-premaster
secret bytes.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep the licensing fingerprint helper compatible with the workspace lint
policy by referring to the range type through core.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep the parent benchmark file-verification helper compiling after
resolving its overlap with the connector capture-path helper.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Require the capture's exact ActiveStage semantic output sequence while
keeping auto-reconnect credential bytes out of the immutable contract.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Require each connector workload to select its matching passive capture
before preparing configuration and validating the transcript.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Reuse the replay measurement as the immutable counter contract and share\nverified capture-path construction between workload preparation paths.\n\nCo-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

ai-reviewed/2 Final automated review completed kind/protocol Affects RDP or related protocol behavior maintainer-required Maintainer review or intervention is required risk/low Self-contained change with no cross-crate behavioral effect scope/tooling Build, CI, release, or developer tooling size/XXL Size: 1300 or more counted lines or 50 or more files triage/overlap This issue or pull request already exists or overlaps

Development

Successfully merging this pull request may close these issues.

3 participants