github-personal/firezone
1
0
Fork 0
mirror of https://github.com/outbackdingo/firezone.git synced 2026-01-27 10:18:54 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(snownet): always update last_activity idle timer (#10000)

Browse Source 
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.
This commit is contained in:
Thomas Eizinger
2025-07-26 01:03:18 +10:00
committed by GitHub
parent d00c3b58cd
commit f55c61c7cb
1 changed files with 28 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files

58
rust/connlib/snownet/src/node.rs
View File

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