fix(apple): increase sensitivity of network reset - #9993
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in Apple platform network connectivity handling where DNS resolution could fail after waking from sleep or network roaming. The fix simplifies the path update logic by removing the "clever" filtering that was causing edge cases.
- Removed custom path comparison logic that filtered path updates based on specific properties
- Simplified network reset to trigger on any path change rather than only "relevant" changes
- Removed conditional DNS resolver fetching to ensure DNS is always updated on path changes
| } | ||
|
|
||
| // Update our path tracker | ||
| self.lastPath = path |
There was a problem hiding this comment.
The lastPath variable is being set but never used for comparison. The code removes the path comparison logic but still tracks lastPath without utilizing it, which suggests incomplete refactoring or dead code.
| self.lastPath = path |
| #endif | ||
|
|
||
| extension Network.NWPath { | ||
| func connectivityDifferentFrom(path: Network.NWPath) -> Bool { |
There was a problem hiding this comment.
The alternative here is to add supportsDNS to the list of connectivity changes we care about. There are lots of other path properties whose changes I'm not sure we need to respond to. This is safer w.r.t. resets, but runs the risk of resetting too often.
There was a problem hiding this comment.
Ok I did the above here. Looks like many new properties are being added to the path struct here in macOS 26 that we won't want to act upon, like link quality changes.
| if lastRelevantPath?.connectivityDifferentFrom(path: path) != false { | ||
| lastRelevantPath = path | ||
|
|
||
| session?.reset("primary network path changed") |
There was a problem hiding this comment.
connlib will do this for you and no-op if they are unchanged. You can save on the JSON processing if you want but it is otherwise save to send the same list again.
There was a problem hiding this comment.
Ok I'll clean that up here.
There was a problem hiding this comment.
Lol, this comment is on the wrong line now? Pretty sure I commented on the DNS resolvers.
e2e696c to
ced966d
Compare
thomaseizinger
left a comment
thomaseizinger
left a comment
There was a problem hiding this comment.
Good catch, I think we need to err on the side of caution here and reset a bit too often rather than not enough.
ced966d to
bcd5fdd
Compare
71b8ac2 to
ee848f3
Compare
ee848f3 to
cdcde23
Compare
| if lastRelevantPath?.connectivityDifferentFrom(path: path) != false { | ||
| lastRelevantPath = path | ||
|
|
||
| session?.reset("primary network path changed") |
There was a problem hiding this comment.
Lol, this comment is on the wrong line now? Pretty sure I commented on the DNS resolvers.
On Apple platforms, we tried to be clever about filtering path updates from the network connectivity change monitor, because there can be a flurry of them upon waking from sleep or network roaming.
However, because of this, we had a bug that could occur in certain situations (such as waking from sleep) where we could effectively "land" on an empty DNS resolver list. This could happen if:
supportsDNSproperty isfalse. This means it hasn't received any resolvers from DHCP yet. We would then setDns with an empty resolver list.supportDNS=true. Since we didn't count this change as a meaningful path change, we skipped thesetDnscall, and connlib would be stuck without DNS resolution.To fix the above, we stop trying to be clever about connectivity changes, and just use
oldPath != path. That will increase reset a bit, but it will now handle other edge cases such as an IP address changing on the primary interface, any other interfaces change, and the like.Fixes #9866