fix(server): measure bandwidth across a single large graphics write

[Greg Lamberson (glamberson)]

Summary

• Continuous bandwidth measurement opened a fixed window of RTT ticks (Start, four ticks, Stop). The client counts the ordinary traffic between Start and Stop (MS-RDPBCGR 3.2.5.14), so a window over a quiet desktop timed idle time: on a 2 ms LAN the server reported a few hundred kbps, and mstsc told the user the network was slow.
• When an EGFX write of at least 10 KiB goes out, the server now sends Bandwidth Measure Start immediately before it and Stop immediately after, on the same stream, so the client times a burst the link actually carried. At most one such measurement per second, and never while one is outstanding. gnome-remote-desktop measures the same way.
• The tick window takes over whenever no bracketed measurement has started for eight RTT ticks, so a session without EGFX, or one whose only large frame was the initial render, keeps measuring.
• A bandwidth measurement the client never answers, or a bracket whose Stop is never sent, is abandoned after the same 30 s as an unanswered RTT probe (expire_stale_probes), instead of stopping bandwidth measurement for the rest of the session. This also covers the tick window, which could already get stuck that way.
• A Bandwidth Measure Results with a zero time delta now counts as one millisecond instead of discarding the figure: The client's timer has millisecond resolution, and a burst on a fast link routinely completes within one. A result with no bytes counted still clears the stored figure.
• New public API: AutoDetectManager::begin_bandwidth_measure and AutoDetectManager::end_bandwidth_measure. Additive; build_bandwidth_measure is unchanged.

Validation

cargo xtask check fmt/lints/tests/typos/locks all pass. Nine new tests in ironrdp-testsuite-core cover the bracket, the size threshold, the pacing, the tick window staying closed while brackets recur and resuming when they stop, the expiry of an unended bracket and of an unanswered measurement, a slow tick window still reaching its Stop, and the zero-delta rule. The existing test that treated a zero time delta as unusable now checks a zero byte count instead.

[github-actions]

The PR brackets large EGFX writes with Bandwidth Measure Start/Stop so the client times a real burst, retires the tick-window measurement after the first bracket, floors zero time deltas at one millisecond, and rejects zero byte counts. Independent inspection confirms the bracketing is protocol-conforming, the zero-delta floor only under-reports, and the in-tree Egfx caller pairs begin/end and aborts on write errors. Published: a one-way latch retires the tick window after a single large write, leaving one-large-write-then-quiet sessions without any bandwidth re-measurement; the new public bracket API can wedge the state machine with no timeout or cancel; and three no-behavior-change compression nits in the new code.

[github-actions]

PR 2021 replaces the fixed tick-window bandwidth measurement (which times idle desktop stretches and reports implausibly low figures) with a Start/data/Stop bracket around EGFX writes of at least 10 KiB, paced to one per second, with the tick window kept as a fallback after eight bracket-free ticks, and adds 30 s expiry for abandoned bandwidth transactions. Independent inspection confirms the mechanism is correct: the bracketed continuous Start/Stop go out back-to-back with matching sequence numbers on the message channel, brackets never overlap and hold off the tick window, the tick counter reset keeps the fallback closed only while brackets recur, expiry covers both a Stop that is never sent and an unanswered measurement without cutting short an open window, and sequence allocation is centralized. The wire-level integration in server.rs matches the existing auto-detect path. Remaining concerns are the unverified zero-timeDelta semantic inversion (which also deviates from the spec-ma…

1. [protocol + skeptical] Zero timeDelta clamped to 1 ms without client-behavior evidence, deviating from the spec-mandated calculation — medium 🟠 ❓ — crates/ironrdp-server/src/autodetect.rs
   measured_bandwidth_kbps inverts the previously tested semantic (zero timeDelta made the result unusable and cleared the stored figure) into clamp-to-1-ms-and-report. The timer-resolution argument is physically plausible and the old rule would discard most bracketed measurements on fast links, but nothing in the repository shows clients never use timeDelta 0 as a no-measurement sentinel, and the PR cites no evidence that gnome-remote-desktop clamps zero deltas. A client that does mean 'invalid' receives a fabricated byte_count*8 kbps figure (capped at u32::MAX, ~4.3 Tbps) advertised in RDP_NETCHAR_RESULTS. The handling also intentionally deviates from the MS-RDPBCGR-mandated (byteCount*8)/timeDelta calculation at both degenerate inputs (zero bytes yields None rather than 0 kbps; zero delta floors to 1 ms where the formula is undefined); the deviation is defensible and partly documented, but the divergence from the MUST clause should be documented against the spec. Missing client-behavior context prevents a firm conclusion.
2. [skeptical] measured_bandwidth_kbps duplicates the PDU crate's computed_bandwidth_kbps, which now has no production callers — low 🟡 — crates/ironrdp-server/src/autodetect.rs
   After this change the server computes bandwidth from the raw Bandwidth Measure Results fields with a private helper, and a search confirms AutoDetectResponse::computed_bandwidth_kbps (ironrdp-pdu/src/rdp/autodetect.rs:613) is referenced only by its own unit tests; the server was its sole production caller. The same wire formula now lives in two crates with deliberately divergent zero handling, so any future correction (rounding, overflow, units) must be found and mirrored by hand, and the two implementations will silently disagree on what a client's result means. The stated goal of avoiding the impossible branch did not require duplication: the special case could wrap a call to the PDU helper or the helper could take the clamped delta.

[Benoît Cortier (CBenoit)]

PR automation is failing because of a picky-krb 0.12.5 incompatibility, fixed on master by #2074. Please rebase on master to fix it.

Update: no rebase needed anymore. picky-krb 0.12.5 was yanked from crates.io (re-released as 0.13.0), so the API check builds again without changes to this branch. PR automation has been re-run here and passes.

[github-actions]

This pull request may overlap with #2012.

Both PRs work in the MS-RDPBCGR auto-detect bandwidth measurement domain: measurement windows of Bandwidth Measure Start/Stop, computing kbps from client results with a one-millisecond floor for zero timeDelta, and expiring unanswered measurements. This PR is the server-side initiator; #2012 is the client-side responder, so the scope is shared but on opposite ends of the same protocol feature.

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

[github-actions]

PR 2021 brackets large EGFX-over-TCP writes with Bandwidth Measure Start/Stop, adds begin/end_bandwidth_measure to AutoDetectManager, computes bandwidth from the client's results with a 1 ms lower bound, centralizes sequence allocation, and expires stuck bandwidth measurements. Independent inspection of pr-head confirms the change is protocol-conformant in framing, sequencing, and pacing, with tests targeting each reverted behavior. All six validated specialist findings check out against the head code: the normative-formula deviation in zero-value handling, the latch guarding an unreachable state (a fresh AutoDetectManager is built per connection at server.rs:2343, the only construction site, contradicting the recorded justification), the duplicated bandwidth arithmetic whose PDU counterpart now has no workspace consumers, the 10 KiB threshold hardcoded in public docs and the external test, the duplicated encode+write trio, and the hand-built results PDU duplicating the new complete_b…

1. [protocol] Bandwidth computation deviates from the normative (byteCount * 8) / timeDelta calculation in zero-value cases — low 🟡 — crates/ironrdp-server/src/autodetect.rs
   MS-RDPBCGR 3.3.5.14 requires the server to calculate bandwidth as (byteCount * 8) / timeDelta. The new measured_bandwidth_kbps deviates at the degenerate inputs: a timeDelta of 0 is clamped to 1 ms, and a byteCount of 0 yields no figure where the formula yields 0. The clamped figure is stored and advertised in the bandwidth field of a Network Characteristics Result, so a value the required calculation does not produce goes on the wire. The spec does not define server behavior for a zero timeDelta or zero byteCount and the clamp is a defensible lower-bound interpretation, so this is a recorded deviation, not a clear violation.
2. [skeptical] measured_bandwidth_kbps reimplements the PDU crate's computed_bandwidth_kbps arithmetic, leaving two definitions of the same figure — low 🟡 — crates/ironrdp-server/src/autodetect.rs
   AutoDetectResponse::computed_bandwidth_kbps (ironrdp-pdu/src/rdp/autodetect.rs:613) already defines bandwidth as byte_count * 8 / time_delta_ms. After this PR the server no longer calls it, so its only workspace consumers are its own unit tests, while the server carries a private copy of the arithmetic in measured_bandwidth_kbps. The deliberate zero-handling differences (zero bytes -> None, zero time -> one-millisecond lower bound) can be layered on the PDU helper for the non-degenerate case. Any future change to the PDU computation, such as saturation or response-type awareness, would then silently diverge from the figure the server reports on the wire and publishes via the autodetect handles.
3. [code-compressor] Extract a shared async helper for encode + write of autodetect PDUs — low 🟡 — crates/ironrdp-server/src/server.rs
   The PR adds two more copies of the encode_autodetect_request -> writer.write_all -> map_err(ServerError::io) trio (the Start and Stop blocks in write_egfx_over_tcp) alongside two pre-existing identical copies in the tick handler (build_netchar_result and build_bandwidth_measure, around lines 3469-3499). A single private async fn write_autodetect_request(writer, pdu, message_channel_id, user_channel_id) -> ServerResult<()> replaces four copies with four one-line calls, removing roughly eight lines and one place per site to get channel ids or the error label wrong, with identical encode/await/error-mapping order so Start-data-Stop ordering and early-return-on-error semantics are preserved. The two pre-existing sites are out of this diff, so adopting the helper there is a small out-of-diff touch; using it only for the new blocks still removes the PR's own duplication but leaves the file inconsistent.

[Greg Lamberson (glamberson)]

On the review's first item, MS-RDPBCGR 3.3.5.14 says the server MUST calculate the bandwidth as (byteCount * 8) / timeDelta. With a timeDelta of 0 that formula divides by zero, so the spec leaves the case open, and I count it as one millisecond, because the client's timer has millisecond resolution and a burst on a fast link routinely completes within one. A byteCount of 0 would give 0, and I return no figure instead, so a measurement that counted nothing clears the stored bandwidth and can't be advertised as a 0 kbps link. Both rules are in the PR body and in the doc of measured_bandwidth_kbps, and zero_time_delta_counts_as_one_millisecond and zero_byte_count_ages_out_a_previous_bandwidth_figure pin them.

[Greg Lamberson (glamberson)]

On the review's second item, the two functions differ on purpose. computed_bandwidth_kbps returns None for a zero timeDelta, computed_bandwidth_zero_delta pins that, and it is public API of ironrdp-pdu, so I don't want this PR to change it. The server needs the opposite for that one input, a one millisecond lower bound, so it keeps its own small function and doesn't layer one rule on the other for a single multiplication and division. The PDU helper stays as it was.

[Greg Lamberson (glamberson)]

On the review's third item, I'm leaving the helper out of this PR. The new Start and Stop blocks follow the pattern the three existing sites in this file already use, and a helper would mean touching those three too, which AGENTS.md asks fixes to avoid ("avoid unrelated refactors"). Using it only for the new blocks would leave the file inconsistent. It would make a reasonable small cleanup of its own.