diff --git a/client/internal/peer/guard/guard.go b/client/internal/peer/guard/guard.go index c1765c462..68d77d318 100644 --- a/client/internal/peer/guard/guard.go +++ b/client/internal/peer/guard/guard.go @@ -104,12 +104,13 @@ func (g *Guard) reconnectLoopWithRetry(ctx context.Context, callback func()) { iceState := &iceRetryState{log: g.log} defer iceState.reset() + netChanged := g.netState.Changed() + for { select { case <-tickerChannel: - // skip attempts while the OS reports no usable network; the ticker - // keeps running so other events remain responsive, and the guard - // resumes via the signal/relay reconnect events once network returns + // skip attempts while the OS reports no usable network; the + // netChanged case below resumes the loop once it returns if !g.netState.IsOnline() { continue } @@ -149,6 +150,23 @@ func (g *Guard) reconnectLoopWithRetry(ctx context.Context, callback func()) { tickerChannel = ticker.C iceState.reset() + case <-netChanged: + // Re-arm for the next transition before acting on this one. + netChanged = g.netState.Changed() + if !g.netState.IsOnline() { + continue + } + // Ticks skipped while offline drove the backoff towards its + // maximum without ever attempting, and left the ICE budget + // frozen — possibly in hourly mode. Recover on our own so the + // peer does not depend on a signal or relay event that never + // comes when both stayed up across the outage. + g.log.Debugf("network is back, reset reconnection ticker") + ticker.Stop() + ticker = g.newReconnectTicker(ctx) + tickerChannel = ticker.C + iceState.reset() + case <-ctx.Done(): g.log.Debugf("context is done, stop reconnect loop") return diff --git a/client/internal/peer/guard/guard_netstate_test.go b/client/internal/peer/guard/guard_netstate_test.go new file mode 100644 index 000000000..601dce33c --- /dev/null +++ b/client/internal/peer/guard/guard_netstate_test.go @@ -0,0 +1,98 @@ +package guard + +import ( + "context" + "sync/atomic" + "testing" + "time" + + log "github.com/sirupsen/logrus" + + "github.com/netbirdio/netbird/client/internal/peer/ice" + "github.com/netbirdio/netbird/client/netstate" +) + +// newTestGuardWithNetState builds a guard with a realistic MaxInterval: the +// backoff must be able to grow well past the outage, as it does in production +// where the timeout is seconds to minutes. +func newTestGuardWithNetState(status connStatusFunc, netState *netstate.State) *Guard { + srw := NewSRWatcher(nil, nil, nil, ice.Config{}) + return NewGuard(log.WithField("test", "guard"), status, 30*time.Second, srw, netState) +} + +// TestGuard_RecoversAfterOfflineToOnline covers a peer that stays disconnected +// across a network outage while neither signal nor relay reports an event — +// both stayed up, as on a short airplane mode toggle over Wi-Fi. +// +// Every tick taken while offline is skipped, but it still advances the +// exponential backoff, so by the time the network returns the next tick can be +// tens of seconds away. Without an explicit reaction to the transition the +// peer waits out that interval for a recovery that could start immediately. +func TestGuard_RecoversAfterOfflineToOnline(t *testing.T) { + netState := netstate.New() + + var attempts atomic.Int32 + g := newTestGuardWithNetState(func() ConnStatus { return ConnStatusDisconnected }, netState) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + // Start from the reconnect ticker (800ms initial interval), the state a + // peer is in after it loses its connection. + go g.Start(ctx, func() { attempts.Add(1) }) + g.SetRelayedConnDisconnected() + + // Let the backoff climb: 0.8s, 1.6s, 3.2s, 6.4s ... every tick is skipped + // while offline, but each one doubles the wait for the next. + netState.Set(false) + time.Sleep(8 * time.Second) + + offlineAttempts := attempts.Load() + if offlineAttempts != 0 { + t.Fatalf("callback ran %d times while offline, want 0", offlineAttempts) + } + + netState.Set(true) + + // The next organic tick is now several seconds out, so anything within + // this window can only come from reacting to the transition itself. + select { + case <-time.After(2 * time.Second): + t.Fatal("peer was not retried within 2s of the network coming back, " + + "with neither a signal nor a relay event to fall back on") + case <-pollUntil(func() bool { return attempts.Load() > 0 }): + } +} + +// TestGuard_OfflineTransitionDoesNotRetry checks the other direction: going +// offline must not itself trigger an attempt. +func TestGuard_OfflineTransitionDoesNotRetry(t *testing.T) { + netState := netstate.New() + + var attempts atomic.Int32 + g := newTestGuardWithNetState(func() ConnStatus { return ConnStatusDisconnected }, netState) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go g.Start(ctx, func() { attempts.Add(1) }) + + netState.Set(false) + time.Sleep(5 * time.Second) + + if got := attempts.Load(); got != 0 { + t.Fatalf("callback ran %d times after going offline, want 0", got) + } +} + +func pollUntil(cond func() bool) <-chan struct{} { + done := make(chan struct{}) + go func() { + for { + if cond() { + close(done) + return + } + time.Sleep(10 * time.Millisecond) + } + }() + return done +} diff --git a/client/netstate/netstate.go b/client/netstate/netstate.go index 138d59926..f67f863ef 100644 --- a/client/netstate/netstate.go +++ b/client/netstate/netstate.go @@ -61,6 +61,20 @@ func (s *State) IsOnline() bool { return s.online } +// Changed returns a channel closed on the next availability transition, for +// callers that already own a select loop and cannot block in Wait. Re-read it +// after every fire: each transition installs a fresh channel. On a nil +// receiver — no State injected — it returns nil, which blocks forever in a +// select, so the caller simply never observes a transition. +func (s *State) Changed() <-chan struct{} { + if s == nil { + return nil + } + s.mu.Lock() + defer s.mu.Unlock() + return s.changed +} + // Wait blocks while the network is offline. It reports whether it had to // wait, so callers can reset their backoff after an outage. It returns early // with the context error when ctx is done. On a nil receiver — no State