fix: reconnect the WireGuard over gRPC tunnel after a failure - #14335
Conversation
| // resettableTimer wraps time.Timer to allow resetting the timer to any duration. | ||
| type resettableTimer struct { | ||
| // ResettableTimer wraps time.Timer to allow arming, re-arming or stopping it. | ||
| type ResettableTimer struct { |
There was a problem hiding this comment.
To keep the changes minimal, I kept this type but fixed its internals/usage. But considered removing it completely, because I'm not sure if t's a good abstraction. If you, the reviewer, would like me to do that, say the word :)
There was a problem hiding this comment.
I think this type tried to wrap old Go problems with resetting timers without consuming from the channel.
And it adds a mutex
There was a problem hiding this comment.
🟡 Changes recommended
The new timer reset logic stops timers without draining the channel on unsuccessful Stop(), which can cause immediate/stale wakeups and break the intended retry delay semantics.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a retry deadlock in the userspace WireGuard-over-gRPC tunnel controller by ensuring the retry timer’s channel remains stable (non-nil) across resets, so a rescheduled retry can wake a select that’s already parked.
Changes:
- Replace the previous “nil-able” timer wrapper with an always-present
ResettableTimer(singletime.Timerfor controller lifetime). - Update the controller loop to use the new timer instance for relay restart scheduling.
- Add unit tests validating that a parked select wakes after arming and that
Reset(0)stops pending firings.
File summaries
| File | Description |
|---|---|
| internal/app/machined/pkg/controllers/siderolink/userspace.go | Introduces the new ResettableTimer behavior and wires it into the relay retry loop to prevent missed retries. |
| internal/app/machined/pkg/controllers/siderolink/userspace_test.go | Adds regression tests covering the “parked select” retry behavior and stopping semantics. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
50c26bd to
17ded15
Compare
When the tunnel connection fails, the retry that should bring it back never runs: 1. Before starting the tunnel relay, the controller cancels the retry with `relayRetryTimer.Reset(0)`, which discards the timer so `relayRetryTimer.C()` becomes a nil channel. 2. The controller then waits on that nil `relayRetryTimer.C()`, which never fires. 3. When the relay fails, it reschedules with `relayRetryTimer.Reset(retryIn)`, which builds a new timer, but the select is still parked on the old nil channel and never sees it. 4. The relay is never restarted, so the tunnel stays down until the node reboots. Keep a single `time.Timer` inside `ResettableTimer` for the life of the controller and only stop or re-arm it, so `relayRetryTimer.C()` is always the same non-nil channel and the rescheduled retry reaches the select. Signed-off-by: Utku Ozdemir <utku.ozdemir@siderolabs.com> Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>
17ded15 to
2e23abc
Compare
|
/m |
When the tunnel connection fails, the retry that should bring it back never runs:
relayRetryTimer.Reset(0), which discards the timer sorelayRetryTimer.C()becomes a nil channel.relayRetryTimer.C(), which never fires.relayRetryTimer.Reset(retryIn), which builds a new timer, but the select is still parked on the old nil channel and never sees it.Keep a single
time.TimerinsideResettableTimerfor the life of the controller and only stop or re-arm it, sorelayRetryTimer.C()is always the same non-nil channel and the rescheduled retry reaches the select.