fix(mobile): serialize controller session teardown

Co-authored-by: Ginder Singh <ginder@windscribe.com>

cmd/ctrld_library/main.go

@@ -1,18 +1,32 @@
 package ctrld_library
 
 import (
+	"sync"
+	"time"
+
 	"github.com/Control-D-Inc/ctrld/cmd/cli"
 )
 
-// Controller holds global state
+// stopTimeout bounds how long Stop waits for ctrld to release its resources.
+const stopTimeout = 15 * time.Second
+
+var (
+	runMobile            = cli.RunMobile
+	checkDeactivationPin = cli.CheckDeactivationPin
+)
+
+// Controller holds global state. Start, Stop and IsRunning are safe for
+// concurrent use. AppCallback and Config must not be accessed concurrently.
 type Controller struct {
+	mu          sync.Mutex
+	pinMu       sync.Mutex // Serializes PIN checks, which use CLI global state.
 	stopCh      chan struct{}
+	doneCh      chan struct{}
 	AppCallback AppCallback
 	Config      cli.AppConfig
 }
 
 // NewController provides reference to global state to be managed by android vpn service and iOS network extension.
-// reference is not safe for concurrent use.
 func NewController(appCallback AppCallback) *Controller {
 	return &Controller{AppCallback: appCallback}
 }
@@ -26,23 +40,36 @@ type AppCallback interface {
 }
 
 // Start configures utility with config.toml from provided directory.
-// This function will block until Stop is called
+// This function blocks until the run exits, including teardown after Stop.
+// A call made while a run is active or stopping returns immediately.
 // Check port availability prior to calling it.
 func (c *Controller) Start(CdUID string, ProvisionID string, CustomHostname string, HomeDir string, UpstreamProto string, logLevel int, logPath string) {
-	if c.stopCh == nil {
-		c.stopCh = make(chan struct{})
-		c.Config = cli.AppConfig{
-			CdUID:          CdUID,
-			ProvisionID:    ProvisionID,
-			CustomHostname: CustomHostname,
-			HomeDir:        HomeDir,
-			UpstreamProto:  UpstreamProto,
-			Verbose:        logLevel,
-			LogPath:        logPath,
-		}
-		appCallback := mapCallback(c.AppCallback)
-		cli.RunMobile(&c.Config, &appCallback, c.stopCh)
+	c.mu.Lock()
+	if c.stopCh != nil {
+		c.mu.Unlock()
+		return
 	}
+	stopCh, doneCh := make(chan struct{}), make(chan struct{})
+	c.stopCh, c.doneCh = stopCh, doneCh
+	c.Config = cli.AppConfig{
+		CdUID:          CdUID,
+		ProvisionID:    ProvisionID,
+		CustomHostname: CustomHostname,
+		HomeDir:        HomeDir,
+		UpstreamProto:  UpstreamProto,
+		Verbose:        logLevel,
+		LogPath:        logPath,
+	}
+	appCallback := mapCallback(c.AppCallback)
+	c.mu.Unlock()
+	defer func() {
+		c.mu.Lock()
+		defer c.mu.Unlock()
+		// Only the completed run releases the guard, even if Stop timed out.
+		close(doneCh)
+		c.stopCh, c.doneCh = nil, nil
+	}()
+	runMobile(&c.Config, &appCallback, stopCh)
 }
 
 // As workaround to avoid circular dependency between cli and ctrld_library module
@@ -63,20 +90,49 @@ func mapCallback(callback AppCallback) cli.AppCallback {
 	}
 }
 
+// Stop requests shutdown after validating pin, unless restart bypasses the check.
+// A zero result means the stop request was accepted, not that teardown finished.
+// After cancellation it waits up to 15 seconds. Callers must check IsRunning
+// after Stop: while it remains true, teardown still owns the session and Start
+// will be rejected. PIN failure codes are returned unchanged.
 func (c *Controller) Stop(restart bool, pin int64) int {
-	var errorCode = 0
+	// Keep this request bound to its original run while PIN validation blocks.
+	c.mu.Lock()
+	stopCh, doneCh := c.stopCh, c.doneCh
+	c.mu.Unlock()
+
 	// Force disconnect without checking pin.
 	// In iOS restart is required if vpn detects no connectivity after network change.
 	if !restart {
-		errorCode = cli.CheckDeactivationPin(pin, c.stopCh)
+		c.pinMu.Lock()
+		errorCode := checkDeactivationPin(pin, stopCh)
+		c.pinMu.Unlock()
+		if errorCode != 0 {
+			return errorCode
+		}
 	}
-	if errorCode == 0 && c.stopCh != nil {
-		close(c.stopCh)
-		c.stopCh = nil
+	if stopCh != nil {
+		c.mu.Lock()
+		select {
+		case <-stopCh:
+		default:
+			close(stopCh)
+		}
+		c.mu.Unlock()
+
+		timer := time.NewTimer(stopTimeout)
+		defer timer.Stop()
+		select {
+		case <-doneCh:
+		case <-timer.C:
+		}
 	}
-	return errorCode
+	return 0
 }
 
+// IsRunning remains true until the run has finished releasing its resources.
 func (c *Controller) IsRunning() bool {
+	c.mu.Lock()
+	defer c.mu.Unlock()
 	return c.stopCh != nil
 }

cmd/ctrld_library/main_test.go

@@ -0,0 +1,275 @@
+package ctrld_library
+
+import (
+	"runtime"
+	"sync"
+	"sync/atomic"
+	"testing"
+	"testing/synctest"
+	"time"
+
+	"github.com/Control-D-Inc/ctrld/cmd/cli"
+)
+
+// Tests replace the first host-touching calls, not the Controller methods.
+// They must not run in parallel because these seams are package globals.
+func stubMobile(t *testing.T, run func(*cli.AppConfig, *cli.AppCallback, chan struct{}), check func(int64, chan struct{}) int) {
+	t.Helper()
+	oldRun, oldCheck := runMobile, checkDeactivationPin
+	runMobile, checkDeactivationPin = run, check
+	t.Cleanup(func() { runMobile, checkDeactivationPin = oldRun, oldCheck })
+}
+
+func startController(c *Controller) <-chan struct{} {
+	done := make(chan struct{})
+	go func() {
+		c.Start("uid", "provision", "hostname", "home", "doh", 2, "log")
+		close(done)
+	}()
+	return done
+}
+
+func TestControllerConcurrentLifecycle(t *testing.T) {
+	synctest.Test(t, func(t *testing.T) {
+		c := NewController(nil)
+		var runs, active atomic.Int32
+		var pinCalls int // Deliberately non-atomic: PIN checks must be serialized.
+		stubMobile(t, func(config *cli.AppConfig, _ *cli.AppCallback, stop chan struct{}) {
+			if active.Add(1) != 1 {
+				t.Error("overlapping mobile runs")
+			}
+			defer active.Add(-1)
+			runs.Add(1)
+			want := cli.AppConfig{CdUID: "uid", ProvisionID: "provision", CustomHostname: "hostname", HomeDir: "home", UpstreamProto: "doh", Verbose: 2, LogPath: "log"}
+			if *config != want {
+				t.Errorf("config = %+v, want %+v", *config, want)
+			}
+			<-stop
+		}, func(_ int64, _ chan struct{}) int {
+			pinCalls++
+			return 0
+		})
+
+		if c.IsRunning() || c.Stop(true, 0) != 0 {
+			t.Fatal("new controller must be stopped")
+		}
+		for cycle := 0; cycle < 25; cycle++ {
+			var starters sync.WaitGroup
+			for i := 0; i < 16; i++ {
+				starters.Add(1)
+				go func() { defer starters.Done(); <-startController(c) }()
+			}
+			synctest.Wait()
+			if !c.IsRunning() || runs.Load() != int32(cycle+1) {
+				t.Fatal("concurrent Starts did not claim exactly one run")
+			}
+			var stoppers sync.WaitGroup
+			for i := 0; i < 16; i++ {
+				stoppers.Add(1)
+				go func() {
+					defer stoppers.Done()
+					c.IsRunning()
+					if code := c.Stop(false, 1234); code != 0 {
+						t.Errorf("Stop = %d", code)
+					}
+					c.IsRunning()
+				}()
+			}
+			stoppers.Wait()
+			starters.Wait()
+			if c.IsRunning() || active.Load() != 0 {
+				t.Fatal("Stop returned before run completion")
+			}
+		}
+		if pinCalls != 25*16 {
+			t.Fatalf("PIN checks = %d, want %d", pinCalls, 25*16)
+		}
+	})
+}
+
+func TestControllerStopWaitsAndTimeoutKeepsGuard(t *testing.T) {
+	synctest.Test(t, func(t *testing.T) {
+		c := NewController(nil)
+		release := make(chan struct{})
+		var runs int
+		stubMobile(t, func(_ *cli.AppConfig, _ *cli.AppCallback, stop chan struct{}) {
+			runs++
+			<-stop
+			<-release
+		}, func(int64, chan struct{}) int { t.Error("restart checked PIN"); return 1 })
+		started := startController(c)
+		synctest.Wait()
+		stopped := make(chan struct{})
+		before := time.Now()
+		go func() {
+			var stoppers sync.WaitGroup
+			for i := 0; i < 8; i++ {
+				stoppers.Add(1)
+				go func() { defer stoppers.Done(); c.Stop(true, 0) }()
+			}
+			stoppers.Wait()
+			close(stopped)
+		}()
+		synctest.Wait()
+		select {
+		case <-stopped:
+			t.Fatal("Stop did not wait for teardown")
+		default:
+		}
+		c.Start("", "", "", "", "", 0, "")
+		if !c.IsRunning() || runs != 1 {
+			t.Fatal("guard released during teardown")
+		}
+		<-stopped // synctest advances virtual time, not wall time.
+		if elapsed := time.Since(before); elapsed != stopTimeout {
+			t.Fatalf("Stop timeout = %v, want %v", elapsed, stopTimeout)
+		}
+		c.Start("", "", "", "", "", 0, "")
+		if !c.IsRunning() || runs != 1 {
+			t.Fatal("timeout released a live run's guard")
+		}
+		close(release)
+		<-started
+		if c.IsRunning() {
+			t.Fatal("completed teardown retained guard")
+		}
+		started = startController(c)
+		synctest.Wait()
+		c.Stop(true, 0)
+		<-started
+		if runs != 2 || c.IsRunning() {
+			t.Fatal("could not restart after timed-out teardown completed")
+		}
+	})
+}
+
+func TestControllerPinBehavior(t *testing.T) {
+	synctest.Test(t, func(t *testing.T) {
+		c := NewController(nil)
+		var runStop chan struct{}
+		var pins []int64
+		stubMobile(t, func(_ *cli.AppConfig, _ *cli.AppCallback, stop chan struct{}) {
+			runStop = stop
+			<-stop
+		}, func(pin int64, stop chan struct{}) int {
+			pins = append(pins, pin)
+			if stop != runStop {
+				t.Error("PIN check received wrong run")
+			}
+			if pin == 1234 {
+				return 0
+			}
+			return 42
+		})
+		if code := c.Stop(false, 9); code != 42 {
+			t.Fatalf("idle Stop PIN result = %d, want 42", code)
+		}
+		started := startController(c)
+		synctest.Wait()
+		if code := c.Stop(false, 8); code != 42 || !c.IsRunning() {
+			t.Fatalf("invalid PIN Stop = %d, running = %v", code, c.IsRunning())
+		}
+		select {
+		case <-runStop:
+			t.Fatal("invalid PIN stopped run")
+		default:
+		}
+		if code := c.Stop(false, 1234); code != 0 {
+			t.Fatalf("valid PIN Stop = %d", code)
+		}
+		<-started
+		started = startController(c)
+		synctest.Wait()
+		c.Stop(true, 9999)
+		<-started
+		if len(pins) != 3 || pins[0] != 9 || pins[1] != 8 || pins[2] != 1234 {
+			t.Fatalf("PIN calls = %v", pins)
+		}
+	})
+}
+
+func TestControllerSlowPinCannotStopReplacement(t *testing.T) {
+	synctest.Test(t, func(t *testing.T) {
+		c := NewController(nil)
+		pinStarted, releasePin := make(chan struct{}), make(chan struct{})
+		var currentStop chan struct{}
+		stubMobile(t, func(_ *cli.AppConfig, _ *cli.AppCallback, stop chan struct{}) {
+			currentStop = stop
+			<-stop
+		}, func(_ int64, stop chan struct{}) int {
+			if stop != currentStop {
+				t.Error("wrong PIN session")
+			}
+			close(pinStarted)
+			<-releasePin
+			return 0
+		})
+		first := startController(c)
+		synctest.Wait()
+		oldStop := make(chan struct{})
+		go func() { c.Stop(false, 1234); close(oldStop) }()
+		<-pinStarted
+		c.Stop(true, 0)
+		<-first
+		second := startController(c)
+		synctest.Wait()
+		close(releasePin)
+		<-oldStop
+		if !c.IsRunning() {
+			t.Fatal("old Stop cleared replacement guard")
+		}
+		select {
+		case <-currentStop:
+			t.Fatal("old Stop cancelled replacement")
+		default:
+		}
+		c.Stop(true, 0)
+		<-second
+	})
+}
+
+func TestControllerMixedConcurrentCalls(t *testing.T) {
+	c := NewController(nil)
+	var active atomic.Int32
+	stubMobile(t, func(config *cli.AppConfig, _ *cli.AppCallback, _ chan struct{}) {
+		if active.Add(1) != 1 {
+			t.Error("overlapping mobile runs")
+		}
+		defer active.Add(-1)
+		runtime.Gosched()
+		if config.CdUID != "uid" {
+			t.Error("active config changed")
+		}
+	}, func(int64, chan struct{}) int { return 0 })
+	var workers sync.WaitGroup
+	for i := 0; i < 12; i++ {
+		workers.Add(1)
+		go func() {
+			defer workers.Done()
+			for j := 0; j < 100; j++ {
+				c.Start("uid", "", "", "", "", 0, "")
+				c.IsRunning()
+				c.Stop(j%2 == 0, 1234)
+			}
+		}()
+	}
+	workers.Wait()
+	if c.IsRunning() || active.Load() != 0 {
+		t.Fatal("concurrent calls left a run active")
+	}
+}
+
+func TestControllerRunReturnReleasesGuard(t *testing.T) {
+	c := NewController(nil)
+	var runs int
+	stubMobile(t, func(*cli.AppConfig, *cli.AppCallback, chan struct{}) { runs++ }, func(int64, chan struct{}) int { return 0 })
+	for i := 0; i < 2; i++ {
+		c.Start("", "", "", "", "", 0, "")
+		if c.IsRunning() {
+			t.Fatal("returned run retained guard")
+		}
+	}
+	if runs != 2 {
+		t.Fatalf("runs = %d, want 2", runs)
+	}
+}