fix: add VPN wake rebind hook#26739
Conversation
johnstcn
left a comment
There was a problem hiding this comment.
Change LGTM but deferring approval to Spike or Ethan.
There was a problem hiding this comment.
Changes look good, and the problem being fixed makes sense to me, though I'm very much surprised I've not experienced nor heard anyone internally complain about the issue being described.
How do we know this is a real issue on macOS? There's that tailscale/tailscale issue, but they've said they won't be fixing it, and also didn't mention whether it was an issue in practice...
Regardless, I've left a comment on coder/coder-desktop-macos#260 for how I think we should handle this on mac.
May be good to hear from Spike on this too.
|
In an unscientific experiment, I slept my MacBook for 3 minutes, 6 minutes and 15 minutes. I'll do an hour+ sleep now, but for each of these, the logs for the Coder Desktop daemon included: and EDIT: Saw these logs after a 5+ hour sleep. Device was connected to the same wifi the entire day. |
|
@ethanndickson If the client synthesizes a major change event on wake then I guess this PR is not needed, I do wonder though if this happens immediately or within a poll (not immediate so might cause some strange UX or VS Code might think it's dead) |
|
Yeah I'm honestly not sure. Maybe it's better to have an explicit rebind event? Right now coder/tailscale is doing a bit more guess work to synthesize a rebind than the solution proposed here. Maybe this event just wasn't firing for the reporting user for whatever reason and so we need to build this? Could be good to check if there's any downsides to spurious rebinds - check if they need debouncing etc |
|
I looked into what spurious rebinds cost. They're safe but not free, so I think we should add a debounce.
What do you think of adding a ~5s debounce in |
That sounds good to me! |
|
https://linear.app/codercom/issue/DEVEX-534/tray-reports-coder-connect-as-healthy-while-tunneldns-is-broken-stale has been sent my way. I haven't spent any time on it, but it's entirely possible these major link changes aren't happening on Windows, and so this mechanism might be more beneficial there? We have logs somewhere for that issue which would let us know. |
I think we can only debounce client events, the synthesized events get propagated from a different code path and thus it would still run anyway 🤔 Also I'm not so sure that this would fix DEVEX-534 since it seems to be in a different path entirely that a reSTUN/reBIND would not fix |
Wake events can arrive in quick succession, e.g. the OS wake notification racing netmon's synthesized link-change event, and each rebind resets peer path trust in magicsock, forcing dual-path sends until paths re-verify. Collapse wakes within 5s into a single rebind so each wake recovers with exactly one rebind. Also drop the unused error_message field from WakeResponse.
a22a29c to
c92f4e5
Compare
ethanndickson
left a comment
There was a problem hiding this comment.
I'm happy with merging this if we think we'll pursue the OS-specific paths in the near future. I think having an explicit / deterministic kick for rebind is good, and it looks like we don't need to be worried about spurious rebinds.
| message WakeRequest {} | ||
|
|
||
| message WakeResponse { | ||
| bool success = 1; |
There was a problem hiding this comment.
Looks unused, always true?
ethanndickson
left a comment
There was a problem hiding this comment.
Forgot to flag this, sorry 😓
After macOS sleep/wake, stale Coder Connect paths can take minutes to recover: magicsock's periodic re-STUN timer stops once the session is idle (and sleep is idle by definition), and a short same-network wake often produces no link-change event. New SSH dials then time out until something else forces path re-discovery (#26736).
This adds a CoderVPN
WakeRequestRPC so Coder Desktop can trigger the existing link-change recovery path (Rebind+ReSTUN) immediately on OS wake, instead of waiting for a timer that isn't running. No polling or health checks are added. Coder Desktop still needs to send the RPC from its wake notification handler: coder/coder-desktop-macos#260.Debouncing
macOS can emit several wake-adjacent notifications in a burst, and each rebind resets peer path trust, briefly forcing traffic onto both the direct path and DERP. The tunnel therefore runs at most one wake-triggered rebind per 5s; debounced wakes still return success.
This only bounds the wake RPC. Rebinds triggered inside magicsock (netmon link changes, periodic re-STUN) are unaffected and continue to serve as the fallback recovery path.
Closes #26736
Generated by Coder Agents.