From f55c61c7cbbb41d51a47b633d33a32992434392f Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Sat, 26 Jul 2025 01:03:18 +1000 Subject: [PATCH] fix(snownet): always update `last_activity` idle timer (#10000) Previously, our idle timer was only driven by incoming and outgoing packets. To detect whether the tunnel is idle, we checked whether either the last incoming or last outgoing packet was more than 20s ago. For one, having two timestamps here is unnecessarily complex. We can simply combine them and always update this timestamp as `last_activity`. Two, recently, we have started to also take into account not only packets but other changes to the tunnel, such as an upsert of the connection or adding new candidate. What we failed to do though, is update these timestamps because their variable name was related to packets and not to any activity. The problem with not updating these timestamps however is that we will very quickly move out of "connected" back to "idle" because the old timestamps are still more than 20s ago. Hence, the previous fixes of moving out of idle on new candidates and connection upsert were ineffective. By combining and renaming the timestamps, it is now much more obvious that we need to update this timestamp in the respective handler functions which then grants us another 20s of non-idling. This is important for e.g. connection upserts to ensure the Gateway runs into an ICE timeout within a short amount of time, should there be something wrong with the connection that the Client just upserted. --- rust/connlib/snownet/src/node.rs | 58 +++++++++++++++----------------- 1 file changed, 28 insertions(+), 30 deletions(-) diff --git a/rust/connlib/snownet/src/node.rs b/rust/connlib/snownet/src/node.rs index f0fc826aa..6f07ba2f9 100644 --- a/rust/connlib/snownet/src/node.rs +++ b/rust/connlib/snownet/src/node.rs @@ -1688,8 +1688,7 @@ enum ConnectionState { /// Our nominated socket. peer_socket: PeerSocket, - last_outgoing: Instant, - last_incoming: Instant, + last_activity: Instant, }, /// We haven't seen application packets in a while. Idle { @@ -1707,11 +1706,9 @@ impl ConnectionState { } match self { - ConnectionState::Connected { - last_incoming, - last_outgoing, - .. - } => Some((idle_at(*last_incoming, *last_outgoing), "idle transition")), + ConnectionState::Connected { last_activity, .. } => { + Some((idle_at(*last_activity), "idle transition")) + } ConnectionState::Connecting { .. } | ConnectionState::Idle { .. } | ConnectionState::Failed => None, @@ -1723,15 +1720,14 @@ impl ConnectionState { TId: fmt::Display, { let Self::Connected { - last_outgoing, - last_incoming, + last_activity, peer_socket, } = self else { return; }; - if idle_at(*last_incoming, *last_outgoing) > now { + if idle_at(*last_activity) > now { return; } @@ -1750,7 +1746,11 @@ impl ConnectionState { { let peer_socket = match self { Self::Idle { peer_socket } => *peer_socket, - Self::Failed | Self::Connecting { .. } | Self::Connected { .. } => return, + Self::Connected { last_activity, .. } => { + *last_activity = now; + return; + } + Self::Failed | Self::Connecting { .. } => return, }; self.transition_to_connected(cid, peer_socket, agent, "upsert", now); @@ -1762,7 +1762,11 @@ impl ConnectionState { { let peer_socket = match self { Self::Idle { peer_socket } => *peer_socket, - Self::Failed | Self::Connecting { .. } | Self::Connected { .. } => return, + Self::Connected { last_activity, .. } => { + *last_activity = now; + return; + } + Self::Failed | Self::Connecting { .. } => return, }; self.transition_to_connected(cid, peer_socket, agent, "new candidate", now); @@ -1774,8 +1778,8 @@ impl ConnectionState { { let peer_socket = match self { Self::Idle { peer_socket } => *peer_socket, - Self::Connected { last_outgoing, .. } => { - *last_outgoing = now; + Self::Connected { last_activity, .. } => { + *last_activity = now; return; } Self::Failed | Self::Connecting { .. } => return, @@ -1790,8 +1794,8 @@ impl ConnectionState { { let peer_socket = match self { Self::Idle { peer_socket } => *peer_socket, - Self::Connected { last_incoming, .. } => { - *last_incoming = now; + Self::Connected { last_activity, .. } => { + *last_activity = now; return; } Self::Failed | Self::Connecting { .. } => return, @@ -1822,8 +1826,7 @@ impl ConnectionState { tracing::debug!(trigger, %cid, "Connection resumed"); *self = Self::Connected { peer_socket, - last_outgoing: now, - last_incoming: now, + last_activity: now, }; apply_default_stun_timings(agent); } @@ -1833,10 +1836,10 @@ impl ConnectionState { } } -fn idle_at(last_incoming: Instant, last_outgoing: Instant) -> Instant { +fn idle_at(last_activity: Instant) -> Instant { const MAX_IDLE: Duration = Duration::from_secs(20); // Must be longer than the ICE timeout otherwise we might not detect a failed connection early enough. - last_incoming.max(last_outgoing) + MAX_IDLE + last_activity + MAX_IDLE } /// The socket of the peer we are connected to. @@ -2097,33 +2100,28 @@ where self.state = ConnectionState::Connected { peer_socket: remote_socket, - last_incoming: now, - last_outgoing: now, + last_activity: now, }; None } ConnectionState::Connected { peer_socket, - last_incoming, - last_outgoing, + last_activity, } if peer_socket == remote_socket => { self.state = ConnectionState::Connected { peer_socket, - last_incoming, - last_outgoing, + last_activity, }; continue; // If we re-nominate the same socket, don't just continue. TODO: Should this be fixed upstream? } ConnectionState::Connected { peer_socket, - last_incoming, - last_outgoing, + last_activity, } => { self.state = ConnectionState::Connected { peer_socket: remote_socket, - last_incoming, - last_outgoing, + last_activity, }; Some(peer_socket)