fix(server): drive clipboard channel timeouts by pierophp · Pull Request #1979 · Devolutions/IronRDP

Piero (pierophp) · 2026-09-18T17:47:35Z

Problem

Cliprdr::drive_timeouts is documented as something the embedder must call from a periodic timer:

Callers must drive this method from a periodic timer in their event loop (e.g., every 5 seconds). The returned PDUs must be sent on the CLIPRDR channel.

ironrdp-client (crates/ironrdp-client/src/rdp.rs) and ironrdp-web (crates/ironrdp-web/src/session.rs) both do. ironrdp-server never calls it from anywhere.

On the server role that means the three sweeps inside drive_timeouts never run:

Outgoing locks are never released. send_lock creates a lock when a file FormatList arrives (2.2.4.1). A later FormatList moves it to Expired — deliberately without sending Unlock, since downloads from the previous clipboard may still be in flight. The Unlock is left to the sweep. With no sweep, it never happens: outgoing_locks grows monotonically until MAX_OUTGOING_LOCKS (100), after which send_lock logs "Too many outgoing locks, skipping automatic lock" and silently stops locking for the rest of the session.
Abandoned file contents requests are never failed. Backends keep waiting on a response that cannot arrive.
Locked file list snapshots are never dropped.

Change

Adds a drive_cliprdr_timeouts arm to client_loop, mirroring the existing send_heartbeats and refresh_auto_reconnect_cookie timers:

CLIPRDR_DRIVE_TIMEOUTS_INTERVAL = 5s, matching the interval the docs suggest.
MissedTickBehavior::Skip, since a stalled write should not produce a burst of back-to-back sweeps for work that is idempotent.
A sweep failure is logged and swallowed. Best-effort maintenance must not disconnect an otherwise healthy session.

What a backend will start seeing. Driving the sweep on the server also activates the two other sweeps inside drive_timeouts, which until now only ran for the client and web roles. Both are governed by the same transfer_timeout (60s by default):

a FileContentsRequest we sent that stays pending for the timeout is dropped, and the backend gets a synthetic error through on_file_contents_response(FileContentsResponse::new_error(stream_id));
a locked file list snapshot with no FileContentsRequest activity for the timeout is dropped, and the backend gets on_unlock(LockDataId(..)).

Neither is new code — both are existing drive_timeouts sweeps that the server simply never reached. But a server CliprdrBackend that treated on_unlock as "the peer sent UnlockData" will now also see it fire on a timeout. That is the intended behaviour, and the point of it (releasing file handles held for a transfer the peer abandoned), but it is worth calling out for backend implementors.

Tests

Two tests in crates/ironrdp-server/src/server.rs:

an_expired_lock_is_released_when_the_sweep_is_driven — negotiates CAN_LOCK_CLIPDATA, feeds a file FormatList (asserting the LockData PDU goes out), expires the lock with a text FormatList, then drives the sweep across a controllable CliprdrBackend::now_ms clock: nothing before the inactivity timeout, an Unlock PDU after it.
a_session_without_a_clipboard_channel_sweeps_quietly — the timer fires for every connection, so a session with no clipboard channel must not be an error path.

Scope note: the tests cover drive_cliprdr_timeouts itself, not the tokio::select! arm that calls it — driving that would need a harness that reaches the active stage, which this crate does not currently have (finalize_timeout.rs stops at the finalize handshake). The arm is six lines mirroring two existing timers.

🤖 Generated with Claude Code

`Cliprdr::drive_timeouts` is documented as something the embedder must call from a periodic timer: it is what sends `Unlock` PDUs for expired clipboard-data locks, answers abandoned file contents requests with synthetic errors, and drops inactive locked file list snapshots. `ironrdp-client` (rdp.rs) and `ironrdp-web` (session.rs) both drive it. `ironrdp-server` never did, so on the server role: - locks created by `send_lock` on a file FormatList were expired by a later FormatList but never released — no `Unlock` PDU was ever sent; - `outgoing_locks` grew monotonically until `MAX_OUTGOING_LOCKS` (100) was reached, after which `send_lock` silently skipped locking for the rest of the session; - file contents requests abandoned by the peer were never failed, leaving backends waiting on a response that could not arrive. Add a `drive_cliprdr_timeouts` arm to `client_loop`, mirroring the existing `send_heartbeats` / `refresh_auto_reconnect_cookie` timers, and swallow sweep failures: best-effort maintenance must not disconnect a healthy session. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Greg Lamberson (glamberson) · 2026-09-20T03:49:03Z

Thanks for this, it's a good catch and a clean fix. I checked the premise against master and it holds. ironrdp-client (rdp.rs) and ironrdp-web (session.rs) both call drive_timeouts on a timer and ironrdp-server never does, so on the server role locks are never released and the 100-lock cap does silently end automatic locking. This affects me directly, since lamco-rdp-server is a long-lived server with file transfer in both directions, so I'm glad to see it fixed.

I also merged the branch with current master and ran the two new tests and the fmt and lints gates, and everything passes. I liked that the expired-lock test drives a controllable clock instead of sleeping, and it's a real test too. When I turn drive_cliprdr_timeouts into a no-op, it fails.

One suggestion for the description. Driving the sweep on the server also turns on two behaviors that already exist for the client and web roles. A file contents request pending for 60 seconds now gets a synthetic error response, and a locked file list snapshot with no requests for 60 seconds is dropped with an on_unlock call to the backend. I think both are right for the server too, but a backend will start seeing them, so a line about it would help reviewers.

A heads-up that it conflicts textually with my #1954 in client_loop, since each adds a writer clone and a select! arm. Keeping both resolves it, so whichever lands second needs a small rebase.

The doc comment on `CLIPRDR_DRIVE_TIMEOUTS_INTERVAL` listed the three sweeps `drive_timeouts` performs but not the two that call into the backend without a PDU from the peer behind them: a file contents request left pending past the transfer timeout is answered with a synthetic error through `on_file_contents_response`, and an inactive locked file list snapshot is dropped with `on_unlock`. Both already ran for the client and web roles; a server backend that read `on_unlock` as "the peer sent UnlockData" starts seeing it fire on a timeout once the server drives the sweep. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Piero (pierophp) · 2026-09-21T19:04:56Z

Thanks for taking the time to verify this independently — especially the no-op mutation. That is the part I could not easily demonstrate myself, and it is the right check for a test whose subject is "something now happens on a timer".

Good call on the two extra behaviours; I have added a paragraph to the description. For the record, both are governed by the same transfer_timeout (60s by default): a FileContentsRequest pending past it is dropped with a synthetic FileContentsResponse::new_error delivered through on_file_contents_response, and a locked file list snapshot with no request activity past it is dropped with on_unlock. Neither is new code, but as you say, a server backend that read on_unlock as "the peer sent UnlockData" will now also see it fire on a timeout, so it belongs in the description rather than only in the cliprdr docs. I also put the same note on the CLIPRDR_DRIVE_TIMEOUTS_INTERVAL doc comment (9eca237), since that is where someone writing a server backend is most likely to look.

On #1954: agreed, it is the writer clone plus the select! arm, and keeping both is the whole resolution. Since #1954 is stacked on #1953 and #1951, this one will probably land first; if it lands the other way around I am happy to rebase.

github-actions

The PR correctly fixes a real gap: nothing in ironrdp-server called CliprdrServer::drive_timeouts, so server-role clipboard locks leaked until MAX_OUTGOING_LOCKS silently disabled locking. The new 5s timer, Skip missed-tick behavior, error-swallowing for sweep failures, and clock-driven tests are sound and mirror the existing heartbeat/auto-reconnect timers; protocol behavior per MS-RDPECLIP is unaffected. One latent robustness issue was refined: the timer treats a missing cliprdr channel ID as a fatal error, inconsistent with the method's best-effort doc and with client_accepted, which skips unjoined channels; however the claimed mass-disconnect is unreachable because a never-joined channel can never have locks or pending requests, so drive_timeouts returns empty messages and the error path is never reached. Two low-severity maintainability findings (a third copy of the SVC send tail, triplicated rationale docs) and a test-coverage gap on the select! arm are accepted.

[code-compressor] Sweep rationale restated in three doc blocks (~40 lines where ~12 suffice) — low 🟡 — crates/ironrdp-server/src/server.rs
The 'what the sweeps do and what a server backend starts seeing' explanation appears nearly verbatim in the CLIPRDR_DRIVE_TIMEOUTS_INTERVAL doc, the drive_cliprdr_timeouts method doc, and the cliprdr_timeout_tests module doc. Keeping the full rationale once (the constant's doc) and reducing the other two to one-line cross-references would cut ~25 lines with zero behavioral impact.

github-actions · 2026-09-30T16:06:32Z

+        let channel_id = self
+            .get_channel_id_by_type::<CliprdrServer>()
+            .ok_or_else(|| ServerError::channel("SVC channel not found"))?;


[skeptical] Timer treats a missing cliprdr channel ID as fatal, inconsistent with best-effort intent — low 🟡 — For a session where a cliprdr factory is configured but the client did not join CLIPRDR, the channel exists in static_channels with no ID (ironrdp-acceptor/src/connection.rs:664-692 attaches IDs only to client-requested channels), so get_svc_processor returns Some while get_channel_id_by_type returns None and this code returns ServerError::channel, which the select! arm propagates as a session-ending error. In practice the path is unreachable: drive_timeouts only emits messages for existing locks/pending requests, which require prior routed PDUs, so msgs is empty and the function returns Ok at the is_empty check. Still, treating the state as fatal contradicts the method doc ('must not disconnect an otherwise healthy session') and client_accepted (server.rs:3572-3574), which skips unjoined channels. Treat None like the missing-processor branch: warn and return Ok.

Agreed. As you note, the path can't be reached today: a channel the client never joined can't hold locks or have pending requests, so drive_timeouts returns no messages and we return before the lookup. Still, a best-effort sweep shouldn't be able to end the session, and client_accepted already skips unjoined channels. Fixed in aa32627: a missing channel ID now logs a warning and returns Ok(()), like the missing-processor branch.

github-actions · 2026-09-30T16:06:32Z

+        let this = Rc::clone(&s);
+        let drive_cliprdr_timeouts = async move {
+            let mut interval = tokio::time::interval(CLIPRDR_DRIVE_TIMEOUTS_INTERVAL);
+            // A stalled write can hold this future past several tick deadlines;
+            // Burst (the default) would then fire the missed ticks back-to-back
+            // for a sweep that is idempotent anyway.
+            interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip);
+            interval.tick().await; // first tick completes immediately
+
+            loop {
+                interval.tick().await;
+                let mut this = this.lock().await;
+                this.drive_cliprdr_timeouts(&mut cliprdr_writer, user_channel_id)
+                    .await?;
+            }
+        };


[skeptical] Only the sweep method is tested; the select! arm and error propagation are not — low 🟡 — The new tests call drive_cliprdr_timeouts directly; no harness reaches the active client_loop stage, so the interval wiring, the arm's propagation of any Err into the select! result, and the constant's actual use are unverified. The arm is precisely where the refined channel-ID error-handling issue manifests. Accepted on the strength of mirroring the existing refresh_auto_reconnect_cookie and send_heartbeats arms.

Acknowledged. This is the gap called out in the description's scope note. Testing the arm needs a harness that reaches the active stage of client_loop, and this crate doesn't have one yet (finalize_timeout.rs stops at the finalize handshake). The arm uses the same pattern as send_heartbeats and refresh_auto_reconnect_cookie, and with the fix above the error path you flagged no longer exists. Building that harness would be a good follow-up, but I'd rather keep it out of this PR.

github-actions · 2026-09-30T16:06:32Z

+        let channel_id = self
+            .get_channel_id_by_type::<CliprdrServer>()
+            .ok_or_else(|| ServerError::channel("SVC channel not found"))?;
+        let data = server_encode_svc_messages(msgs, channel_id, user_channel_id).map_err(ServerError::encode)?;
+        writer
+            .write_all(&data)
+            .await
+            .map_err(|e| ServerError::io("write_all", e))?;


[code-compressor] Channel-lookup/encode/write tail is a third copy of identical handler code — low 🟡 — These lines duplicate the same tail in ServerEvent::Clipboard (~2955-2963) and ServerEvent::Rdpdr (~2927-2935): get_channel_id_by_type -> ok_or_else(ServerError::channel) -> server_encode_svc_messages -> write_all. A small shared helper would replace all three with one call, preserving error mapping and ordering. Optional consolidation touching pre-existing handlers.

Fair point about the duplication. I've left it out on purpose: the other two copies are in the existing ServerEvent::Clipboard and ServerEvent::Rdpdr handlers, and changing them would mix a refactor into a bugfix. The copy in this PR has also drifted from the other two since the fix above, because it skips a missing channel ID instead of returning ServerError::channel. A shared helper would therefore have to choose between the two behaviors. That's worth doing, but in its own PR.

…-cliprdr-timeouts # Conflicts: # crates/ironrdp-server/src/server.rs

A configured clipboard channel the client never joined has no channel ID. Treat that like client_accepted does and skip it instead of returning a channel error from a best-effort maintenance sweep. Also trim the test module doc to a cross-reference to the constant's doc. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Piero (pierophp) · 2026-09-30T17:03:54Z

[code-compressor] Sweep rationale restated in three doc blocks

Partly addressed in aa32627. The full explanation now lives only in the CLIPRDR_DRIVE_TIMEOUTS_INTERVAL doc, and the test module doc is a one-line pointer to it. The doc on drive_cliprdr_timeouts was already a one-line pointer. It only adds one thing of its own, that failures are logged and swallowed, which belongs next to the code that does it.

github-actions

The PR correctly fixes a real gap: ironrdp-server never drove CliprdrServer::drive_timeouts, so server-role outgoing clipboard locks leaked until the 100-lock cap silently disabled locking, and abandoned file-contents requests were never failed. The 5s timer with MissedTickBehavior::Skip mirrors the existing send_heartbeats and refresh_auto_reconnect_cookie arms; write failures propagate (dead connection) while best-effort sweep failures are logged and swallowed, and the unjoined-channel path skips like client_accepted. All three specialist findings are valid and accepted: (1) activating the sweep makes the server evict incoming locked file-list snapshots after the transfer timeout without the peer's Unlock PDU, a defensible resource-leak tradeoff but a deviation from MS-RDPECLIP 3.1.5.3.2 retention semantics worth flagging; (2) the select! arm that actually makes the fix work is untested (acknowledged scope gap); (3) CapturingWriter is byte-for-byte duplicated from cliprdr_error_test…

github-actions · 2026-09-30T17:27:56Z

+    /// Drives the clipboard channel's time-based cleanup.
+    ///
+    /// See [`CLIPRDR_DRIVE_TIMEOUTS_INTERVAL`]. A cleanup failure is logged and
+    /// swallowed: the sweep is best-effort maintenance, and failing it must not
+    /// disconnect an otherwise healthy session.
+    async fn drive_cliprdr_timeouts(
+        &mut self,
+        writer: &mut impl FramedWrite,
+        user_channel_id: u16,
+    ) -> ServerResult<()> {
+        let Some(cliprdr) = self.get_svc_processor::<CliprdrServer>() else {
+            return Ok(());
+        };
+
+        let msgs = match cliprdr.drive_timeouts() {
+            Ok(msgs) => Vec::from(msgs),
+            Err(error) => {
+                warn!(%error, "Clipboard timeout cleanup failed");
+                return Ok(());
+            }
+        };
+
+        if msgs.is_empty() {
+            return Ok(());
+        }
+
+        // A configured channel the client never joined has no ID. Skip it like
+        // `client_accepted` does rather than end the session over it.
+        let Some(channel_id) = self.get_channel_id_by_type::<CliprdrServer>() else {
+            warn!("Clipboard channel not joined, dropping timeout cleanup messages");
+            return Ok(());
+        };
+        let data = server_encode_svc_messages(msgs, channel_id, user_channel_id).map_err(ServerError::encode)?;
+        writer
+            .write_all(&data)
+            .await
+            .map_err(|e| ServerError::io("write_all", e))?;
+
+        Ok(())
+    }


[protocol] Timer-driven sweep evicts incoming locked file lists before the peer's Unlock PDU — medium 🟠 — Wiring drive_timeouts into the server role activates its sweep, which drops an incoming locked file-list snapshot after the transfer timeout of inactivity and only notifies the local backend. MS-RDPECLIP 3.1.5.3.2 requires File Stream data covered by a received Lock Clipboard Data PDU to be stored until an Unlock Clipboard Data PDU is received, and 3.1.5.4.6 requires a File Contents Request PDU carrying that clipDataId to be serviced from the locked data. After eviction, a late request referencing the still-locked ID can only be answered with CB_RESPONSE_FAIL even though the peer never released the lock. Verified against drive_timeouts_impl (ironrdp-cliprdr/src/lib.rs:1292-1311). The deviation is bounded by the 60s default transfer_timeout and trades unconditional retention for avoiding unbounded memory growth on abandoned transfers; the outgoing-lock Unlock PDUs the same timer emits are conformant under 3.1.5.3.3, and the synthetic file-contents error stays backend-local with no wire footprint.

github-actions · 2026-09-30T17:27:56Z

+        let drive_cliprdr_timeouts = async move {
+            let mut interval = tokio::time::interval(CLIPRDR_DRIVE_TIMEOUTS_INTERVAL);
+            // A stalled write can hold this future past several tick deadlines;
+            // Burst (the default) would then fire the missed ticks back-to-back
+            // for a sweep that is idempotent anyway.
+            interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip);
+            interval.tick().await; // first tick completes immediately
+
+            loop {
+                interval.tick().await;
+                let mut this = this.lock().await;
+                this.drive_cliprdr_timeouts(&mut cliprdr_writer, user_channel_id)
+                    .await?;
+            }
+        };


[skeptical] The select! arm that wires the timer into client_loop is untested — low 🟡 — Both new tests call drive_cliprdr_timeouts directly, so the interval loop, Skip missed-tick behavior, mutex acquisition, and ? propagation that ends the session on write failure are never executed by a test. A regression such as dropping the arm from the select! or accidentally reverting to burst behavior would pass the suite. Mitigating context: the arm mirrors the existing send_heartbeats and refresh_auto_reconnect_cookie timers, the PR's scope note acknowledges the gap (no harness reaches the active stage of client_loop), and the direct tests do cover the driven method including the expired-lock and no-channel paths.

github-actions · 2026-09-30T17:27:56Z

+    #[derive(Default)]
+    struct CapturingWriter(Vec<u8>);
+
+    impl FramedWrite for CapturingWriter {
+        type WriteAllFut<'write>
+            = core::future::Ready<std::io::Result<()>>
+        where
+            Self: 'write;
+
+        fn write_all<'a>(&'a mut self, buf: &'a [u8]) -> Self::WriteAllFut<'a> {
+            self.0.extend_from_slice(buf);
+            core::future::ready(Ok(()))
+        }
+    }


[code-compressor] CapturingWriter test helper duplicated from cliprdr_error_tests — low 🟡 — The new cliprdr_timeout_tests module declares CapturingWriter (struct plus FramedWrite impl) byte-for-byte identical to the existing declaration in cliprdr_error_tests (head lines 5872-5885). Hoisting it into a shared cfg(test) support module would declare it once, delete the duplicate, and prevent drift if the helper changes. Test-only with zero runtime impact; optional cleanup rather than a defect.

This was referenced Sep 18, 2026

feat(clipboard): keep a paste running when the clipboard is replaced MuNeNiCK/hypr-rdp#104

Draft

Clipboard: replacement during an active transfer can invalidate queued file requests MuNeNiCK/hypr-rdp#97

Open

Piero (pierophp) marked this pull request as ready for review September 21, 2026 18:55

Piero (pierophp) deployed to llm-providers September 21, 2026 18:56 — with GitHub Actions Active

github-actions Bot added needs-review A human reviewer is the current next actor risk/unknown Risk could not be determined automatically; needs maintainer-level scrutiny size/M Size: up to 449 counted lines and 10 files; exceeds S in either measure labels Sep 21, 2026

Piero (pierophp) deployed to llm-providers September 21, 2026 19:05 — with GitHub Actions Active

Devolutions (Devolutions) deleted a comment from github-actions Bot Sep 30, 2026

github-actions Bot reviewed Sep 30, 2026

View reviewed changes

github-actions Bot added ai-reviewed/1 One automated review completed needs-author-action The pull request author is the current next actor and removed needs-review A human reviewer is the current next actor labels Sep 30, 2026

Piero (pierophp) and others added 2 commits September 30, 2026 13:59

Merge remote-tracking branch 'upstream/master' into feat/server-drive…

c735597

…-cliprdr-timeouts # Conflicts: # crates/ironrdp-server/src/server.rs

Piero (pierophp) deployed to llm-providers September 30, 2026 17:04 — with GitHub Actions Active

github-actions Bot removed needs-author-action The pull request author is the current next actor triage/overlap Possible overlap with another pull request; advisory only labels Sep 30, 2026

github-actions Bot reviewed Sep 30, 2026

View reviewed changes

github-actions Bot added ai-reviewed/2 Final automated review completed needs-author-action The pull request author is the current next actor and removed ai-reviewed/1 One automated review completed labels Sep 30, 2026

Conversation

Piero (pierophp) commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Change

Tests

Uh oh!

Greg Lamberson (glamberson) commented Sep 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Piero (pierophp) commented Sep 21, 2026

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

Piero (pierophp) Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

Piero (pierophp) Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

Piero (pierophp) Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

Piero (pierophp) commented Sep 30, 2026

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

This branch was successfully deployed

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Piero (pierophp) commented Sep 18, 2026 •

edited

Loading

Greg Lamberson (glamberson) commented Sep 20, 2026 •

edited

Loading