From f164c9a9dfecc68a8fcc5176e974fd5a52bceeb4 Mon Sep 17 00:00:00 2001 From: RustDesk <71636191+rustdesk@users.noreply.github.com> Date: Wed, 9 Sep 2026 17:56:41 +0800 Subject: [PATCH] Dead peer recovery (#16117) * webrtc: recover from a silent peer in about 8s instead of 30s A controlled peer that is killed, switched away by a user switch, or rebooted leaves no trace on a UDP transport: there is no reset to receive, so the session sees silence, and only the 30s inactivity timeout ends it. By then the remote machine may have finished rebooting and be reachable again, while the user has been watching a frozen frame the whole time and is then told the peer reset the connection. ICE already knows sooner. It reports Disconnected about 5s after it stops hearing from the peer, from its own task, so it stays accurate even while this loop is busy sending. That state is transient by design - a Wi-Fi roam or a sleep/wake recovers from it - so it is treated as suspicion, not as death: three more seconds with the transport receiving nothing, and the session reconnects. Receive progress cancels the suspicion, so a peer that is merely slow, or one ICE was late to clear, is not dropped. This only reaches the existing recovery sooner; it does not replace it. The first reconnect goes out immediately and, if it fails, falls into the same retry the UI already applies to any unexpected disconnect. The restart reconnect event is reused deliberately: it is what asks for exactly that, with no error dialog in front of it, and the UI shows "Connecting..." for it rather than anything about restarting. Its five-minute grace stays reserved for a restart the user actually asked for - silence is no evidence of a reboot. The 30s timeout is unchanged and still backs every transport. TCP and WebSocket are untouched. The controlled side is untouched: it detects a dead controller on the same 30s, which wastes some capture but nothing a user sees. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019aokqJuhjvB3kijXtAg5Ns * kcp: recover from a silent peer on the endpoint's own clock KCP is the other transport with nothing to receive when the peer dies, and it was the slower of the two: its endpoint reaps a connection only after 60s without a packet, which is past the 30s inactivity timeout above it, so in practice nothing but that timeout ever noticed. The endpoint already tracks when each connection last heard from its peer and now exposes it, so this reads that rather than anything derived from the session loop - it keeps answering while that loop is busy sending. Its liveness ping now goes out about every 2s rather than every 10s, so silence means the peer rather than an idle link, and eight seconds of it is several missed pings. Same threshold and the same recovery as the WebRTC half, so a user sees the same thing on either transport. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019aokqJuhjvB3kijXtAg5Ns * review: time the inactivity window off receive progress, bound the parting send Two things the review found, both on the controlling side. The 30s inactivity window still ran off completed messages alone, so the probe added for the fast path did not fix what it was added for: a message larger than the transport's fragment size yields nothing until its last fragment, and a peer sending one steadily was still timed out mid-transfer. It is now timed off whichever is later, a completed message or receive progress. Transports that report no progress leave that at its starting value, so nothing else moves. The parting close-reason send for KCP waited on send capacity with no deadline of its own, and a queue a dead peer will never drain held the finished session's thread until the endpoint reaped the connection a minute later. Bounded once the peer has been declared gone. Still attempted rather than skipped: if the loss was one-way the peer does receive it, and drops its side immediately instead of waiting out its own timeout - which is also the one case where the note below resolves itself. Recorded from the same review, for the case none of this targets - a peer that is alive behind a path that broke for five to ten seconds and then healed. Giving up cannot deliver a close there, because the path is still down at that moment, so the controlled side keeps the old connection until its own 30s expires. For up to twenty of those seconds it holds two authorised connections: its connection manager lists both, and the stale one reports a growing delay that pins the shared frame rate low for the new one. Input is unaffected throughout and both recover once the stale connection goes, so this trades twenty-two seconds of a frozen, uncontrollable session for a controllable one that looks wrong for a while. Closing the displaced connection is controlled-side work and belongs with the rest of it, not here. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019aokqJuhjvB3kijXtAg5Ns * review: reject a disconnected cached session, tidy the detector hbb_common: `is_reusable_for` now also rejects a session ICE reports Disconnected, so a caller is not handed one that already carries the hint; and the receive-progress test no longer races `next()` against a sleeping sibling. Here: the `is_some()` guard on the progress comparison was dead, since a transport answers `None` for its whole life and `None != None` is already false. The parting-send deadline is a `Duration` like every other constant around it rather than bare milliseconds. And the comments are cut back to what is not already evident from the code they sit on. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019aokqJuhjvB3kijXtAg5Ns * review: keep the legacy UI's retrying error when the peer goes silent `restarting-show` is a Flutter control event; Sciter has no case for it and falls through to a plain dialog, which `check_if_retry` marks non-retryable because its type is not `error`. So on that build the new detector would have replaced a timeout that reconnects on its own after 30s with a dialog waiting for a click at 8s - a regression for the one path this was meant to shorten. Send it the message the timeout already sends, so its behaviour is unchanged apart from arriving sooner. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019aokqJuhjvB3kijXtAg5Ns * review: keep the 30s watchdog hard, and let Android's picker hold the reconnect Timing the watchdog off receive progress gave away its upper bound. A fragment bumps the counter as it arrives, ahead of the framing checks that would reject it, so a peer sending one `FRAG_MORE` every twenty seconds and never a `FRAG_END` refreshed the deadline forever while the reassembly buffer grew toward `MAX_FRAME_LENGTH`, a gigabyte away. What it bought - a clipboard image that takes longer than thirty seconds to arrive is not a dead peer - is a pre-existing problem that predates this branch and can be fixed on its own. Receive progress goes back to the one job it was added for, which needs no deadline of its own: telling a transport that has gone quiet from one that is still delivering, so ICE's disconnected hint is not acted on mid-transfer. The Android document picker suppresses a `Connection Error` while it is open and remembers to reconnect once it closes. The peer-gone break reconnects under `restarting-show` with a `Connecting...` title, which matched neither half of that test, so an eight-second stall behind an open picker - Doze and background throttling produce them - threw a dialog up behind the picker and lost the deferred reconnect. It is now named there by its own title rather than by its type: an explicitly restarted remote device sends the same type from a path this leaves alone, on every transport, and deferring that one too would be a change to sessions this has no business touching. The two limits are still not hard upper bounds, and the comment saying so was wrong about why. A send is awaited inline in this loop, so one in progress delays the tick that checks them - bounded on WebRTC by the timeout the stream was built with, not bounded at all on KCP, whose framed stream is constructed with none. The 30s watchdog beside it shares the loop and the same delay. Left alone deliberately. `restarting-show` reconnects without the backoff its `restarting` sibling uses, which can loop while each round gets far enough to establish a session and then loses the transport within eight seconds; a cooldown there would also delay the recovery this exists for when a peer really does come back, and the loading it shows can be cancelled. And the KCP limit reads an accumulated silence rather than a transient hint, so unlike the WebRTC grace it needs no second sample to confirm - one would only move eight seconds to nine. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019aokqJuhjvB3kijXtAg5Ns --------- Co-authored-by: Claude Opus 5 --- Cargo.lock | 2 +- flutter/lib/models/model.dart | 6 ++++- libs/hbb_common | 2 +- src/client/io_loop.rs | 49 +++++++++++++++++++++++++++++++++++ src/kcp_stream.rs | 19 +++++++++++--- 5 files changed, 71 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 999fc644a..e58ffd397 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4280,7 +4280,7 @@ dependencies = [ [[package]] name = "kcp-sys" version = "0.1.0" -source = "git+https://github.com/rustdesk-org/kcp-sys?branch=rustdesk-patches#023a0065398968989f2ddfcf5cc72bb886d02675" +source = "git+https://github.com/rustdesk-org/kcp-sys?branch=rustdesk-patches#938eda3e5e9757a612385503af7a6cb1189b2cdd" dependencies = [ "anyhow", "auto_impl", diff --git a/flutter/lib/models/model.dart b/flutter/lib/models/model.dart index 56c4462ca..c7a48280a 100644 --- a/flutter/lib/models/model.dart +++ b/flutter/lib/models/model.dart @@ -896,9 +896,13 @@ class FfiModel with ChangeNotifier { final text = evt['text']; final link = evt['link']; + // The peer-gone detector reconnects under `restarting-show` rather than an error title, so + // it needs naming here too. By its own title, not the type: an explicitly restarted remote + // device reaches the same type from a path this change does not touch. if (isAndroid && _androidDocumentPickerActive && - title == 'Connection Error') { + (title == 'Connection Error' || + (type == 'restarting-show' && title == 'Connecting...'))) { _androidDocumentPickerInterruptedConnection = true; return; } diff --git a/libs/hbb_common b/libs/hbb_common index 55395c6fc..29cf7cbe4 160000 --- a/libs/hbb_common +++ b/libs/hbb_common @@ -1 +1 @@ -Subproject commit 55395c6fcbcb8dd4bc8d4e7ab4d7d7c8d1789b43 +Subproject commit 29cf7cbe4d38ce36020749f713fb066299f02431 diff --git a/src/client/io_loop.rs b/src/client/io_loop.rs index a49a2d841..9de20588e 100644 --- a/src/client/io_loop.rs +++ b/src/client/io_loop.rs @@ -15,6 +15,16 @@ use crate::{ // Restart msgbox text is kept as a legacy UI fallback; Flutter handles the type as a control event. const RESTART_REMOTE_DEVICE_NO_DATA_TIMEOUT: Duration = Duration::from_secs(5); const KCP_CLOSE_REASON_FLUSH_DELAY: Duration = Duration::from_millis(30); +// Deadline for the parting close-reason send once the peer is presumed gone; KCP waits for send +// capacity with no deadline of its own. +const KCP_CLOSE_REASON_GONE_DEADLINE: Duration = Duration::from_millis(500); +// Grace after ICE reports Disconnected, which it does ~5s after it stops hearing from the peer, +// for ~8s in total. Disconnected is transient by design, so this waits out a Wi-Fi roam or a +// sleep/wake rather than acting on the first hint. +const WEBRTC_SUSPECT_GRACE: Duration = Duration::from_secs(3); +// KCP gets no such hint, only how long since a packet arrived; its endpoint pings an idle peer +// about every 2s, so this is several missed pings, and matches the 8s WebRTC arrives at. +const KCP_PEER_SILENCE_LIMIT: Duration = Duration::from_secs(8); #[cfg(feature = "unix-file-copy-paste")] use crate::{clipboard::try_empty_clipboard_files, clipboard_file::unix_file_clip}; use base::{ @@ -247,6 +257,9 @@ impl Remote { let _keep_it = client::hc_connection(feedback, rendezvous_server, token).await; let mut last_recv_time = Instant::now(); + let mut webrtc_suspect_since: Option = None; + let mut last_rx_progress = peer.rx_progress(); + let mut peer_gone = false; loop { tokio::select! { @@ -313,6 +326,37 @@ impl Remote { self.handler.msgbox("restarting-show", "Restarting remote device", "Connection in progress. Please wait.", ""); break; } + let rx_progress = peer.rx_progress(); + // `None` for transports that report none, and it never changes for a + // given one, so they are inert here. + let progressed = rx_progress != last_rx_progress; + last_rx_progress = rx_progress; + if peer.webrtc_disconnected() && !progressed { + webrtc_suspect_since.get_or_insert_with(Instant::now); + } else { + webrtc_suspect_since = None; + } + // Neither limit is a hard upper bound. A send is awaited inline in + // this loop, so one in progress delays this tick - bounded on WebRTC + // by the timeout the stream was built with, not bounded at all on + // KCP. The 30s watchdog above shares the loop and the same delay. + peer_gone = webrtc_suspect_since + .map_or(false, |since| since.elapsed() >= WEBRTC_SUSPECT_GRACE) + || kcp + .as_ref() + .and_then(|k| k.peer_silent_for()) + .map_or(false, |silent| silent >= KCP_PEER_SILENCE_LIMIT); + if peer_gone { + log::info!("Peer stopped answering, reconnecting"); + #[cfg(feature = "flutter")] + self.handler.msgbox("restarting-show", "Connecting...", "Connection in progress. Please wait.", ""); + // Sciter knows no `restarting-show` and would show a dialog that + // waits for a click, where the timeout this arrives ahead of is + // retryable and reconnects on its own. Keep that message for it. + #[cfg(not(feature = "flutter"))] + self.handler.msgbox("error", "Connection Error", "Timeout", ""); + break; + } let elapsed = fps_instant.elapsed().as_millis(); if elapsed < 1000 { continue; @@ -358,6 +402,11 @@ impl Remote { s.send(()).ok(); } if kcp.is_some() { + // Attempted rather than skipped even here: if the loss was one-way the peer + // does get it, and drops its side instead of waiting out its own timeout. + if peer_gone { + peer.set_send_timeout(KCP_CLOSE_REASON_GONE_DEADLINE.as_millis() as u64); + } // Send the close reason if it hasn't been sent yet, as KCP cannot detect the socket close event. self.send_close_reason(&mut peer, "kcp").await; // KCP does not send messages immediately, so wait to ensure the last message is sent. diff --git a/src/kcp_stream.rs b/src/kcp_stream.rs index 2c4cfe4bd..50a00296f 100644 --- a/src/kcp_stream.rs +++ b/src/kcp_stream.rs @@ -8,14 +8,15 @@ use hbb_common::{ tokio_util, ResultType, Stream, }; use kcp_sys::{ - endpoint::KcpEndpoint, + endpoint::{ConnId, KcpEndpoint}, packet_def::{KcpPacket, KcpPacketHeader}, stream, }; use std::{net::SocketAddr, sync::Arc}; pub struct KcpStream { - _endpoint: KcpEndpoint, + endpoint: KcpEndpoint, + conn_id: ConnId, stop_sender: Option>, } @@ -41,6 +42,14 @@ impl KcpStream { } } + /// How long since a valid packet was last received from the peer, or `None` once the + /// connection is gone. Answered by the KCP endpoint's own tasks, not by the session's read + /// loop, so it stays meaningful while that loop is busy sending a large message; and the + /// endpoint pings an idle peer often enough that silence here means the peer, not quiet. + pub fn peer_silent_for(&self) -> Option { + self.endpoint.peer_silent_for(&self.conn_id) + } + fn create_framed(stream: stream::KcpStream, local_addr: Option) -> Stream { Stream::Tcp(FramedStream( tokio_util::codec::Framed::new(DynTcpStream(Box::new(stream)), BytesCodec::new()), @@ -77,7 +86,8 @@ impl KcpStream { if let Some(stream) = stream::KcpStream::new(&endpoint, conn_id) { Ok(( Self { - _endpoint: endpoint, + endpoint, + conn_id, stop_sender: Some(stop_sender), }, Self::create_framed(stream, udp_socket.local_addr().ok()), @@ -108,7 +118,8 @@ impl KcpStream { if let Some(stream) = stream::KcpStream::new(&endpoint, conn_id) { Ok(( Self { - _endpoint: endpoint, + endpoint, + conn_id, stop_sender: Some(stop_sender), }, Self::create_framed(stream, udp_socket.local_addr().ok()),