fix(egfx): respect client frame acknowledgement limits

[Piclaw (piclaw-bot)]

A client advertising a one-frame ACK window can currently be sent frames using the server's larger configured window. An AVC producer also receives the same None for a full window as for a permanent submission error, which makes ordinary backpressure difficult to handle without replacing the encoder.

This PR is based directly on master and is independent of #1413. It contains no credential, binder or acceptor changes.

• Apply the nonzero core Frame Acknowledge capability as a ceiling on the operator-configured EGFX window, including legacy graphics factories. Configured zero remains unlimited only when the client has no ceiling; resize preserves the negotiated ceiling.
• Reject unknown/stale ACKs before they can suspend or mutate tracking. Preserve valid suspension/resumption for frames issued in the current channel generation, and clear channel-owned state on close.
• Report client queue depth as Unavailable: our client counts frames, while the protocol field measures bytes. This is distinct from Suspend.
• Add typed AVC420, AVC444 and AVC444v2 submission errors, including Backpressured, while retaining the existing Option methods as compatibility wrappers. Mixed-codec send APIs are unchanged.

WRDP exposed the configured-three/client-one case during desktop dragging. These changes let the producer retain its encoder and retry/recover after an ACK instead of treating backpressure as encoder failure.

Focused checks passed locally (Rust 1.96; the repository pins 1.94.1):

cargo test -p ironrdp-egfx --lib                               # 55 passed
cargo test -p ironrdp-testsuite-core --test integration_tests_core -- egfx::server  # 92 passed
cargo test -p ironrdp-server --lib --features egfx -- frame_ack # 2 passed
cargo test -p ironrdp-server --lib --features egfx -- gfx::tests # 2 passed
cargo fmt --all
git diff --check

Regressions cover window/zero semantics, activation clamps, resize, legacy factories, stale suspend/resume ACKs across close/reopen, restored tracking after valid resume, and typed rejection without queued output. A read-only review caught the legacy-factory and suspended-ACK resume paths; both are corrected and covered here.

Happy to adjust the additive error API to the project's preferred shape during review. This PR can be reviewed and merged separately from the session-binding work.

[Copilot]

🟡 Changes recommended

Channel reopening loses the negotiated ceiling, and stale same-generation ACKs can incorrectly resume tracking.

2 open findings

• Reject stale ACKs for frames acknowledged before suspension · New
• Preserve client frame ceiling across EGFX channel resets · New

What changed in this PR

This PR improves EGFX frame-flow control and AVC submission error handling.

Changes:

• Applies client frame-ack limits to server backpressure.
• Hardens ACK tracking and reports unavailable queue depth correctly.
• Adds typed AVC submission errors and regression tests.

File	Description
crates/​ironrdp-egfx/​src/​server.rs	Implements frame limits, ACK tracking, and typed errors.
crates/​ironrdp-egfx/​src/​client.rs	Reports queue depth as unavailable.
crates/​ironrdp-server/​src/​server.rs	Applies the client capability limit.
crates/​ironrdp-server/​src/​gfx.rs	Retains handles for legacy factories.
crates/​ironrdp-testsuite-core/​tests/​egfx/​server.rs	Adds flow-control regressions.

🧠 Review effort: Balanced

[github-actions]

The PR's core changes are sound: clamping the in-flight window to the client's non-zero Frame Acknowledge capability (including legacy factories and resize), validating ACKs before they mutate flow-control state, generation-scoped suspend/resume, typed AVC submission errors, and the conformant client QueueDepth::Unavailable report all check out against the head code and tests. Two real defects remain: a suspend (0xFFFFFFFF) Frame Acknowledge can now be dropped by the new is_ready gate or the tracked-frame requirement in acknowledge(), regressing the previously unconditional MS-RDPEGFX 3.2.5.13 suspend transition and risking a flow-control stall; and reset_channel_state clears the client-advertised ceiling while the server automation only re-applies it during activation, so a mid-session channel reopen can revert to an oversized window. The remaining accepted findings are low-severity maintainability issues (duplicated submission gates and QoE branch, write-only frames_queued counter, …

1. [protocol] Suspend Frame Acknowledge can be dropped, regressing the unconditional suspend requirement — medium 🟠 — crates/ironrdp-egfx/src/server.rs
   MS-RDPEGFX 3.2.5.13 requires the suspend transition whenever queueDepth is 0xFFFFFFFF. The new is_ready gate drops any ACK (including suspend) outside the Ready state, e.g. a suspend PDU arriving after resize() moved to Resizing and cleared tracking; FrameTracker::acknowledge likewise returns None before the suspend check unless the frameId is currently tracked. Pre-PR the suspend transition was unconditional, so this is a regression: the server never suspends, resumes sending, and stalls waiting for acknowledgments a suspended client will not send. Suspend PDUs naming untracked frame IDs are also ignored.
2. [skeptical] Legitimate ACKs for suspension-period frames are logged as protocol violations after resume — low 🟡 — crates/ironrdp-egfx/src/server.rs
   Frames sent while ack_suspended are intentionally untracked, and resume_from_untracked_ack only fires while still suspended. Once the client opts back in, its ACKs for those suspension-period frames fall through both acknowledge() and resume_from_untracked_ack into the warn! 'Ignoring unknown or stale frame acknowledgement' branch — operator-visible noise on every suspend/resume cycle, and QoE/total_acked skip those frames. Pre-PR these logged at debug as a benign resume/stale case. Flow control is unaffected.
3. [skeptical] frames_queued counter is maintained but never observed after the QueueDepth fix — low 🟡 — crates/ironrdp-egfx/src/client.rs
   Reporting QueueDepth::Unavailable removed the only reader of frames_queued. The field is still initialized, incremented on frame receipt, decremented on completion and reset on state changes, with only test assertions left reading it. Dead bookkeeping that should be deleted with its updates, or the change invites a future caller to reintroduce the frames-vs-bytes confusion this change documents.
4. [skeptical + code-compressor] avc420_submission_state and avc444_submission_state duplicate the submission gate — low 🟡 — crates/ironrdp-egfx/src/server.rs
   The two new check methods run the identical is_ready -> codec-support -> should_backpressure -> UnknownSurface sequence, differing only in the codec predicate and error variant (~15 duplicated lines), and both try_send callers repeat the Backpressured-to-QoE mapping. A single parameterized gate (passing the codec Result to preserve NotReady-over-unsupported precedence) keeps readiness, backpressure and surface checks, and error precedence in sync in one place — historically where the AVC420/AVC444 paths drifted.

Push a commit after addressing these findings. If no code change is needed, you may resolve inline threads and comment @github-actions review-ready to request human review.

[Piclaw (piclaw-bot)]

Addressed the review findings in ebc27e1. Thanks for checking the suspension and reopen edge cases.

• Resume eligibility now includes only frames outstanding when suspension began or issued during that suspension. Already-acknowledged duplicates, including out-of-order holes, cannot resume tracking or alter queue depth. Eligibility is cleared on resume/reset; repeated suspend requests preserve the active pause's eligibility.
• The core client ceiling now survives DVC reopen and resize. Initial connection activation replaces it, including clearing an old constraint when the next client supplies no nonzero limit. Reactivation preserves the existing ceiling. The reopen regression no longer manually reapplies the clamp.
• Per MS-RDPEGFX 2.2.2.13 and 3.2.5.13, a valid suspend request is honoured independently of readiness/RTT sample tracking. It still names a frame issued in the active generation; fabricated and previous-generation IDs do not mutate that generation. Added coverage for an already-acknowledged frame requesting suspension outside Ready, repeated suspend, and wrapping IDs.
• Removed unused client queue bookkeeping, shared the backpressure/QoE result handling, removed the overwritten reset-state assignment, and changed benign ignored-ACK logs to debug.

The public Option-returning methods are retained deliberately for source compatibility with external callers.

Focused tests: 57 EGFX unit tests, 94 EGFX server regressions, and 4 clamp/factory tests passed. Local EGFX Clippy passes with command-local exceptions for two unchanged upstream diagnostics on installed Rust 1.96; no source lint suppression was added. Pinned CI will run on this push.

[github-actions]

The PR is a well-tested, well-motivated flow-control rework: it applies the client's Frame Acknowledge capability as a ceiling on the EGFX window, validates stale/duplicate ACKs against channel generations before they can mutate state, and replaces ambiguous Option returns with typed FrameSubmissionError. I independently verified the generation-scoped suspend/resume logic (including u32 wrapping and clear()-on-close behavior), the zero-window semantics, and the activation-path clamp application; no correctness defect was found in the new state machine itself. Four publishable concerns remain: an unresolved wire-value ambiguity where the repo's own removed comment equated Unavailable with 0xFFFFFFFF while the pdu maps Suspend to 0xFFFFFFFF, which the new suspend machinery entrenches and which must be reconciled with the MS-RDPEGFX special queueDepth values; a duplicated, production-dead public clamp API on a published crate; a reactivation pass that re-negotiates capabilities but delib…

Push a commit after addressing these findings. If no code change is needed, you may resolve inline threads and comment @github-actions review-ready to request human review.

[github-actions]

The PR correctly applies the client's core Frame Acknowledge capability as a ceiling on the EGFX window (including legacy factories), converts submission APIs to typed errors with Option compatibility wrappers, fixes the client's queue-depth semantics to Unavailable (bytes, not frames), and adds generation-scoped suspend/resume validation. The core design is sound and well-tested, but one protocol defect stands out: the new generation-boundary gate in suspend_from_untracked_ack drops a conformant SUSPEND frame acknowledge whose frameId predates the last tracker clear (e.g. across a resize), which can deadlock flow control. Remaining items are low-severity maintainability concerns: an uncalled new public preflight method, a now-unreachable direct-registration fallback in egfx_channel_id, and three small test duplications.

1. [skeptical] Direct GraphicsPipelineServer fallback in egfx_channel_id is now unreachable in production — low 🟡 — crates/ironrdp-server/src/gfx.rs
   build_server_and_handle routes every factory, including legacy ones, through GfxDvcBridge, and server.rs registration is the only production EGFX registration, so the get_channel_id_by_type::<GraphicsPipelineServer>() fallback in egfx_channel_id is exercised only by the egfx_is_found_whichever_way_it_was_registered test. This dead path is exactly the direct-registration style whose staleness this PR fixes elsewhere; remove it or document why direct registration must remain routable to keep the legacy bug from resurfacing.

Push a commit after addressing these findings. If no code change is needed, you may resolve inline threads and comment @github-actions review-ready to request human review.

[github-actions]

This pull request may overlap with #2104.

Both PRs touch the same GfxServerFactory construction path in crates/ironrdp-server/src/gfx.rs and server.rs: this PR adds build_server_and_handle around the build_server_with_handle/build_gfx_handler fallback that PR 2104 rewrites, and both alter how the EGFX server and its shared handle are built and attached per connection.

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).

[Rui Carmo (rcarmo)]

@github-actions review-ready