From 89c6e84a41486469c3244d7caed56d3e5db3e1c8 Mon Sep 17 00:00:00 2001 From: Zoltan Papp Date: Fri, 28 Aug 2026 09:33:08 +0200 Subject: [PATCH] [client, ios] Fix context cancellation during restart (#7329) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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. --- client/ios/NetBirdSDK/client.go | 122 ++++++++++++++++++++++++-------- 1 file changed, 93 insertions(+), 29 deletions(-) diff --git a/client/ios/NetBirdSDK/client.go b/client/ios/NetBirdSDK/client.go index 8373e498a..bbbb969c9 100644 --- a/client/ios/NetBirdSDK/client.go +++ b/client/ios/NetBirdSDK/client.go @@ -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.