Skip to content

fix(session): preserve bulk compression across reactivation#1474

Draft
irvingouj@Devolutions (irvingoujAtDevolution) wants to merge 2 commits into
masterfrom
fix/bulk-compression-reactivation
Draft

fix(session): preserve bulk compression across reactivation#1474
irvingouj@Devolutions (irvingoujAtDevolution) wants to merge 2 commits into
masterfrom
fix/bulk-compression-reactivation

Conversation

@irvingoujAtDevolution

Copy link
Copy Markdown
Contributor

Any session that reactivates (Deactivate All → re-activate) loses bulk decompression and dies right after. Windows consoles reactivate right after logon, and compression is on by default, so this hits pretty easily.

The reactivation path rebuilt the FastPath processor with bulk_decompressor: None. After that every compressed update got parsed as a raw bitmap:

Received compressed FastPath data but no decompressor is configured
BitmapData decode NotEnoughBytes: received 1662, expected 17134

Fix: ActiveStage keeps the negotiated compression type and gets a reactivate(...) that rebuilds the processor with a fresh decompressor. Native client, web, FFI and the e2e test all go through it now instead of hand-building a processor. The FFI one was the same bug — worth noting since RDM hits that path.

All additive, no API break (compression_type was already on ActiveStageBuilder).

Testing

Hyper-V console on my lab box, default config (K64), before/after — first frame then dead vs. a session that keeps rendering. Compression ratio was ~24x on the initial paint, so this isn't a rare edge.

Also ran a normal RDP session to a Windows Server VM to be sure nothing regressed.

cargo check/clippy clean (session, client, web wasm32, ffi), fmt clean, testsuite-core 949 + testsuite-extra 21 green.

The Deactivation-Reactivation Sequence rebuilt the FastPath processor with
`bulk_decompressor: None`, silently dropping the negotiated bulk decompressor.
Every compressed FastPath update received afterwards then failed to decode (the
still-compressed bytes were parsed as a raw bitmap), terminating the session.
Windows consoles reactivate right after logon, so any connection negotiating
bulk compression (the default) was affected.

`ActiveStage` now retains the negotiated compression type and exposes
`reactivate(...)`, which rebuilds the FastPath processor with a freshly created
decompressor. The native client, web client, FFI bindings, and e2e test drive
reactivation through it instead of hand-building a processor with no
decompressor.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Preserves negotiated bulk compression when rebuilding session processors after reactivation.

Changes:

  • Adds ActiveStage::reactivate with decompressor recreation.
  • Migrates native, web, FFI, and e2e paths to the new API.
  • Retains compression configuration within ActiveStage.

Reviewed changes

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

Show a summary per file
File Description
crates/ironrdp-session/src/active_stage.rs Implements compression-aware reactivation.
crates/ironrdp-client/src/rdp.rs Uses the unified reactivation API.
crates/ironrdp-web/src/session.rs Uses the unified reactivation API.
ffi/src/session/mod.rs Preserves compression through FFI reactivation.
crates/ironrdp-testsuite-extra/tests/e2e.rs Updates the reactivation test path, but does not exercise compression.

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

bulk_decompressor: None,
}
.build(),
stage.reactivate(
Address review feedback on the reactivation fix: link the spec section from
`reactivate`, stop `set_share_id` from claiming it must be called during a
Deactivation-Reactivation Sequence (it no longer does), and warn on
`set_fastpath_processor` that a hand-built processor is how bulk decompression
gets silently dropped. The FFI method keeps its name for ABI compatibility, so
say what it actually does now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants