[client, ios] Fix context cancellation during restart (#7329)

* fix(mobile): stop the client synchronously so a restart cannot inherit a cancelled context

Original finding
----------------
A user reported that leaving home and switching from wifi to cellular killed
all Internet traffic until NetBird was turned off. A debug bundle captured the
failure (iOS, CLI 0.75.0, self-hosted management, generated 2026-08-18 01:17;
the incident is at 2026-08-17 22:37:38-51 UTC).

The bundle shows the whole sequence:

  22:37:38.255  management sync stream drops (keepalive ACK timeout)
  22:37:43.670  Swift: "Network type changed: wifi -> cellular" -> schedules a
                restart with a 1s debounce
  22:37:44.737  Go: "ensuring wg interface is removed, Netbird engine context
                cancelled" - engineCtx dies, every peer gets context canceled
  22:37:49.910  iface.go:238 "failed to remove WireGuard interface utun6:
                timeout when waiting for interface utun6 to be removed"
                -> the teardown stretches out for ~5s
  22:37:50.710  Swift: "restartClient: starting client", needsLogin=false
                (so this is NOT a login expiry)
  22:37:51.013  Go: connect.go:476 "exiting client retry loop due to
                unrecoverable error: context canceled" - the OLD run dies here
  22:37:51.333  Go: grpc.go:135 "failed creating connection to Management
                Service: context canceled" - the NEW start, 2ms after the old
                run finally exited
  22:37:51.334  Swift: "restartClient: start failed" -> widget disconnected
  then nothing for 15 minutes

The tunnel stayed installed with no engine behind it, so every packet was
black-holed. status.txt, generated ~14 hours later, still reads Management:
Disconnected / Signal: Disconnected / Peers count: 0/0 - the client never
recovered on its own.

Root cause
----------
Client.Stop() cancelled a shared ctxCancel field and returned immediately,
without waiting for the run loop to exit. The Swift stop{} completion handler
therefore fired while the Go teardown was still running (stretched out by the
utun6 removal timeout), and the start that followed landed on a context that
the outgoing run was about to cancel.

Two further paths wrote the same shared field. IsLoginRequired() and
LoginForMobile() each overwrote c.ctxCancel, so any call to them during a live
session discarded the running engine's cancel function. restartClient() calls
needsLoginCached() on exactly this path.

Changes
-------
- Stop() now drives the stored ConnectClient: ConnectClient.Stop() cancels the
  run context and blocks on runExited, so the caller's completion handler only
  fires once the run loop has really finished. The ctxCancel path stays as a
  fallback for when no ConnectClient exists yet (e.g. during LoginForMobile).
- Run() owns its cancel in a local variable, so a concurrent call that
  overwrites the shared field can no longer cancel this run's context through
  the deferred cleanup.
- IsLoginRequired() and LoginForMobile() use local cancels and leave the shared
  field alone. LoginForMobile's cancel moves into the deferred cleanup of the
  goroutine that outlives the call, so the OAuth token wait is not cut short.
- The Android SDK gets the same treatment. The structural defect is identical
  there, but the trigger is absent: Android has no automatic engine restart on
  a network type change, and no interface-removal timeout to stretch the
  teardown. This part is preventive, not a fix for an observed failure.

* fix(mobile): do not let a superseded startup publish its client

Review found a window the previous commit left open. Run stored its cancel
function and only published the ConnectClient later, after loading config and
constructing the client. A Stop landing inside that window found no
ConnectClient, cancelled the run and returned immediately. A new Run could then
publish its own client, and the cancelled older run — still executing — would
overwrite it with a client that was already being torn down. The next Stop
stopped that stale client and left the live one running with nothing tracking
it.

Runs now carry a generation. Run claims one before doing any work and publishes
its client only while the generation is still current; a superseded run returns
without touching the shared state. Stop bumps the generation, so any startup
still in flight is invalidated, then cancels it and waits for the run to exit
before returning (20s cap so a wedged teardown cannot block the caller
forever).

setState is gone: publishState replaces it at both call sites on each platform.

* fix(ios): add a non-waiting Stop for callers on a deadline

Stop now waits for the run loop to exit, which is what a restart needs but
wrong for stopTunnel: iOS gives NEPacketTunnelProvider only a few seconds
there before it kills the extension, and the wait can run to its 20s cap.
Waiting past the deadline earns a SIGKILL, so the next start inherits a dirty
state instead of the orderly shutdown the wait was meant to buy.

StopWithoutWait tears the client down and returns. ConnectClient.Stop blocks on
runExited with no cap of its own, so the non-waiting path runs it detached
rather than only skipping the runDone wait.

Android keeps a single blocking Stop: it has no equivalent deadline.

* fix(mobile): guard the run lifecycle with a single lock

Stop and beginRun each touched the same lifecycle state across two locks in
sequence: take stateMu, release it, then take ctxCancelLock. A run starting in
that gap installed its own cancel before Stop reached it, so Stop cancelled the
fresh run and left its own target running — the same class of defect this branch
exists to fix, this time in the locking rather than the state.

ctxCancel moves into the stateMu group, and both sides take their snapshot in
one critical section. ctxCancelLock then guarded nothing and is gone.

* fix(mobile): drop the run-generation machinery for a serialized lifecycle

The platform callers (Swift/Kotlin) always stop before starting and coalesce
restarts, so the generation counter guarded against call patterns that cannot
occur. Replace it with a single-run contract:

- startRun refuses a second Run while the previous one has not exited
- finishRun clears the published state on every exit path, including errors
- Stop cancels and waits for the run loop with a bounded timeout; it no
  longer calls ConnectClient.Stop, whose wait is unbounded
- concurrent Stops wait on the same exit channel instead of returning early
- a superseded startup no longer reports a clean nil exit

* revert(android): drop the run lifecycle changes

Android does not have the defect this PR fixes. On ux/ios-style-redesign the
EngineRestarter is gone: network changes are handled as events instead of an
engine restart, so nothing stops the client and starts it again.

The remaining stop() callers are all final teardowns on the main thread with a
framework deadline - the stop-engine broadcast receiver, onDestroy, onRevoke and
the binder's stopEngine. A Stop that waits for the run loop would risk an ANR
there for a race that cannot occur, so the fix stays iOS-only.

* fix(ios): make loginComplete race-free

The OAuth goroutine spawned by LoginForMobile sets loginComplete after the
call has returned to Swift, while the Swift side polls IsLoginComplete and
later calls ClearLoginComplete from its own thread. The plain bool made all
three unsynchronized: the store may never become visible to the poller, and
a Clear racing the store can be lost, leaving a stale true that makes the
next login look already complete.

Switch the field to atomic.Bool. It is a standalone flag rather than part of
the run lifecycle that stateMu guards, and it has to stay readable while the
login goroutine is still in flight.
This commit is contained in:
Zoltan Papp
2026-08-28 09:33:08 +02:00
committed by GitHub
parent 6620219939
commit 89c6e84a41

View File

@@ -4,12 +4,14 @@ package NetBirdSDK
import (
"context"
"errors"
"fmt"
"net/netip"
"os"
"sort"
"strings"
"sync"
"sync/atomic"
"time"
log "github.com/sirupsen/logrus"
@@ -37,6 +39,8 @@ const (
AnonymizeLevelStrict = nbAnonymize.LevelStrictString
)
var errClientAlreadyRunning = errors.New("client is already running")
// RouteListener export internal RouteListener for mobile
type NetworkChangeListener interface {
listener.NetworkChangeListener
@@ -74,15 +78,13 @@ type Client struct {
cacheDir string
logFilePath string
recorder *peer.Status
ctxCancel context.CancelFunc
ctxCancelLock *sync.Mutex
deviceName string
osName string
osVersion string
networkChangeListener listener.NetworkChangeListener
onHostDnsFn func([]string)
dnsManager dns.IosDnsManager
loginComplete bool
loginComplete atomic.Bool
// netMgr outlives engine restarts: it mirrors the OS connectivity, not
// the engine lifecycle. Run injects its state and sweeper into each new
// ConnectClient.
@@ -90,9 +92,16 @@ type Client struct {
// preloadedConfig holds config loaded from JSON (used on tvOS where file writes are blocked)
preloadedConfig *profilemanager.Config
// stateMu guards the run lifecycle as one unit: the cancel installed by
// the current run, the channel it closes on exit, and the state it
// published. One run at a time: startRun refuses a second Run while the
// previous one has not exited, and the platform serializes Stop before
// Start, so no generation tracking is needed.
stateMu sync.RWMutex
connectClient *internal.ConnectClient
config *profilemanager.Config
runDone chan struct{}
ctxCancel context.CancelFunc
}
// NewClient instantiate a new Client
@@ -107,7 +116,6 @@ func NewClient(cfgFile, stateFile, cacheDir, logFilePath, deviceName string, osV
osName: osName,
osVersion: osVersion,
recorder: recorder,
ctxCancelLock: &sync.Mutex{},
networkChangeListener: networkChangeListener,
dnsManager: dnsManager,
netMgr: netevents.NewManager(recorder),
@@ -156,17 +164,21 @@ func (c *Client) Run(fd int32, interfaceName string, envList *EnvList) error {
c.recorder.UpdateManagementAddress(cfg.ManagementURL.String())
c.recorder.UpdateRosenpass(cfg.RosenpassEnabled, cfg.RosenpassPermissive)
var ctx context.Context
//nolint
ctxWithValues := context.WithValue(context.Background(), system.DeviceNameCtxKey, c.deviceName)
//nolint
ctxWithValues = context.WithValue(ctxWithValues, system.OsNameCtxKey, c.osName)
//nolint
ctxWithValues = context.WithValue(ctxWithValues, system.OsVersionCtxKey, c.osVersion)
c.ctxCancelLock.Lock()
ctx, c.ctxCancel = context.WithCancel(ctxWithValues)
defer c.ctxCancel()
c.ctxCancelLock.Unlock()
runCtx, runCancel := context.WithCancel(ctxWithValues)
defer runCancel()
done, err := c.startRun(runCancel)
if err != nil {
return err
}
defer c.finishRun(done)
ctx := runCtx
// No login pre-flight here. The engine's own loginToManagement (connect.go) performs
// the authoritative Login immediately before the first Sync, so a LoginSync() call at
@@ -215,16 +227,40 @@ func (c *Client) NotifyNetworkChange() {
c.netMgr.NotifyNetworkChange()
}
// Stop the internal client and free the resources
// Stop cancels the running client and waits for the run loop to exit, so a
// caller that restarts immediately cannot race the outgoing teardown.
func (c *Client) Stop() {
c.ctxCancelLock.Lock()
defer c.ctxCancelLock.Unlock()
if c.ctxCancel == nil {
done := c.cancelRun()
if done == nil {
return
}
c.ctxCancel()
c.setState(nil, nil)
select {
case <-done:
case <-time.After(stopRunWaitTimeout):
log.Warnf("Stop: timed out waiting for the run loop to exit")
}
}
// StopWithoutWait cancels the running client without waiting for the run loop.
// Use it where the caller is on a deadline the wait could overrun, such as
// NEPacketTunnelProvider.stopTunnel, which iOS gives only a few seconds
// before it kills the extension.
func (c *Client) StopWithoutWait() {
c.cancelRun()
}
func (c *Client) cancelRun() chan struct{} {
c.stateMu.RLock()
done := c.runDone
cancel := c.ctxCancel
c.stateMu.RUnlock()
if cancel != nil {
cancel()
}
return done
}
// DebugBundle generates a debug bundle, uploads it and returns the upload key.
@@ -376,16 +412,14 @@ func (c *Client) IsLoginRequiredCached() bool {
}
func (c *Client) IsLoginRequired() bool {
var ctx context.Context
//nolint
ctxWithValues := context.WithValue(context.Background(), system.DeviceNameCtxKey, c.deviceName)
//nolint
ctxWithValues = context.WithValue(ctxWithValues, system.OsNameCtxKey, c.osName)
//nolint
ctxWithValues = context.WithValue(ctxWithValues, system.OsVersionCtxKey, c.osVersion)
c.ctxCancelLock.Lock()
defer c.ctxCancelLock.Unlock()
ctx, c.ctxCancel = context.WithCancel(ctxWithValues)
ctx, cancel := context.WithCancel(ctxWithValues)
defer cancel()
var cfg *profilemanager.Config
var err error
@@ -433,17 +467,22 @@ func (c *Client) IsLoginRequired() bool {
// loginForMobileAuthTimeout is the timeout for requesting auth info from the server
const loginForMobileAuthTimeout = 30 * time.Second
const stopRunWaitTimeout = 20 * time.Second
func (c *Client) LoginForMobile() string {
var ctx context.Context
//nolint
ctxWithValues := context.WithValue(context.Background(), system.DeviceNameCtxKey, c.deviceName)
//nolint
ctxWithValues = context.WithValue(ctxWithValues, system.OsNameCtxKey, c.osName)
//nolint
ctxWithValues = context.WithValue(ctxWithValues, system.OsVersionCtxKey, c.osVersion)
c.ctxCancelLock.Lock()
defer c.ctxCancelLock.Unlock()
ctx, c.ctxCancel = context.WithCancel(ctxWithValues)
ctx, cancel := context.WithCancel(ctxWithValues)
loginDone := false
defer func() {
if !loginDone {
cancel()
}
}()
// Use DirectUpdateOrCreateConfig to avoid atomic file operations (temp file + rename)
// which are blocked by the tvOS sandbox in App Group containers
@@ -470,7 +509,9 @@ func (c *Client) LoginForMobile() string {
}
// This could cause a potential race condition with loading the extension which need to be handled on swift side
loginDone = true
go func() {
defer cancel()
tokenInfo, err := oAuthFlow.WaitToken(ctx, flowInfo)
if err != nil {
log.Errorf("LoginForMobile: WaitToken failed: %v", err)
@@ -487,18 +528,18 @@ func (c *Client) LoginForMobile() string {
log.Errorf("LoginForMobile: Login failed: %v", err)
return
}
c.loginComplete = true
c.loginComplete.Store(true)
}()
return flowInfo.VerificationURIComplete
}
func (c *Client) IsLoginComplete() bool {
return c.loginComplete
return c.loginComplete.Load()
}
func (c *Client) ClearLoginComplete() {
c.loginComplete = false
c.loginComplete.Store(false)
}
func (c *Client) GetRoutesSelectionDetails() (*RoutesSelectionDetails, error) {
@@ -718,13 +759,36 @@ func (c *Client) DeselectRoute(id string) error {
return nil
}
// setState stores the running engine state so DebugBundle can reuse the live
// config and ConnectClient. It is cleared on Stop.
func (c *Client) setState(cfg *profilemanager.Config, cc *internal.ConnectClient) {
func (c *Client) startRun(cancel context.CancelFunc) (chan struct{}, error) {
c.stateMu.Lock()
defer c.stateMu.Unlock()
if c.runDone != nil {
return nil, errClientAlreadyRunning
}
done := make(chan struct{})
c.runDone = done
c.ctxCancel = cancel
return done, nil
}
func (c *Client) finishRun(done chan struct{}) {
c.stateMu.Lock()
c.connectClient = nil
c.config = nil
c.runDone = nil
c.ctxCancel = nil
c.stateMu.Unlock()
close(done)
}
func (c *Client) setState(cfg *profilemanager.Config, cc *internal.ConnectClient) {
c.stateMu.Lock()
c.config = cfg
c.connectClient = cc
c.stateMu.Unlock()
}
// stateSnapshot returns the current config and ConnectClient under the lock.