feat(connlib): always use all candidates - #9979
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
thomaseizinger
force-pushed
the
fix/update-idle-peer-socket
branch
from
July 23, 2025 07:19
93f0bde to
4291d68
Compare
thomaseizinger
force-pushed
the
fix/update-idle-peer-socket
branch
from
July 23, 2025 07:32
c89f0ba to
0da7b63
Compare
thomaseizinger
marked this pull request as ready for review
July 23, 2025 07:35
thomaseizinger
force-pushed
the
fix/update-idle-peer-socket
branch
from
July 23, 2025 07:41
e1ff028 to
03e8c5e
Compare
thomaseizinger
enabled auto-merge
July 23, 2025 07:43
thomaseizinger
commented
Jul 23, 2025
Comment on lines
-874
to
-877
| // In case we were using the relays, all connections will be cut and require us to make a new one. | ||
| if self.drop_direct_client_traffic { | ||
| self.client.exec_mut(|client| client.reset_connections()); | ||
| } |
Member
Author
There was a problem hiding this comment.
As we can see from the removal of these lines, we can now successfully migrate connections to a new relay 🥳
thomaseizinger
changed the title
jamilbk
approved these changes
Jul 24, 2025
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Jul 24, 2025
Now that we are capable of migrating a connection to another relay with #9979, our test suite exposed an edge-case: If we are in the middle of migrating a connection, it could be that the idle timer triggers because we have not seen any application traffic in the last 20s. Moving to idle mode drastically reduces the number of STUN bindings we send and if this happens whilst we are still checking candidates, the nomination doesn't happen in time for our boringtun handshake to succeed. Thus, we add a condition to our idle timer to not trigger unless ICE has completed and reports us as `connected`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In #6876, we added functionality that would only make use of new remote candidates whilst we haven't nominated a socket yet with the remote. The reason for that was because in the described edge-case where relays reboot or get replaced whilst the client is partitioned from the portal (or we experience a connection hiccup), only one of the two peers, i.e. Client or Gateway would migrate to the new relay, leaving the other one in an inconsistent state.
Looking at recent customer logs, I've been seeing a lot of these messages:
For this particular customer, these are then very quickly followed by ICE timeouts, leaving the connection unusable.
Considering that, I no longer think that the above change was a good idea and we should instead always make use of all candidates that we are given. What we are seeing is that in deployment scenarios where the latency link between Client and Gateway is very short (5-10ms) yet the latency to the portal is longer (~30-50ms), we trigger a race condition where we are temporarily nominating a peer-reflexive candidate pair instead of a regular one. This happens because with such a short latency link, Client and Gateway are faster in sending back and forth several STUN bindings than the control plane is in delivering all the candidates.
Due to the functionality added in #6876, this then results in us not accepting the candidates. It further appears that a nominated peer-reflexive candidate does not provide a stable connection which is why we then run into an ICE timeout, requiring Firezone to establish a new connection only to have the same thing happen again.
This is very disruptive for the user experience as the connection only works for a few moments at a time.
With #9793, we have actually added a feature that is also at play here. Now that we don't immediately act on an ICE timeout, it is actually possible for both Client and Gateway to migrate a connection to a different relay, should the one that they are using get disconnected. In #9793, we added a timeout of 2s for this.
To make this fully work, we need to patch str0m to transition to
Checkingearly. Presently, str0m would directly transition fromDisconnectedtoConnectedin this case which in some of the high-latency scenarios that we are testing in CI is not enough to recover the connection within 2s. By transitioning toCheckingearly, we abort this timer.Related: algesten/str0m#676