From f9c9cff50f6285e47a455c0eeadb2d89a0cf5048 Mon Sep 17 00:00:00 2001 From: atavism Date: Thu, 30 Jul 2026 11:31:40 -0700 Subject: [PATCH 1/9] Fix macOS VPN startup reentry --- lantern-core/mobile/mobile.go | 18 +++++----- lantern-core/mobile/mobile_test.go | 33 +++++++++++++++++ macos/Runner/VPN/VPNBase.swift | 56 ++++++++++++++++++++++++++++- macos/Runner/VPN/VPNManager.swift | 44 +++++++++++------------ macos/RunnerTests/RunnerTests.swift | 28 ++++++++++++++- 5 files changed, 145 insertions(+), 34 deletions(-) diff --git a/lantern-core/mobile/mobile.go b/lantern-core/mobile/mobile.go index bab8a15c02..bd7645e047 100644 --- a/lantern-core/mobile/mobile.go +++ b/lantern-core/mobile/mobile.go @@ -30,7 +30,7 @@ var ( errLanternNotReady = errors.New("radiance not initialized") ipcServer *ipc.Server - ipcClient *ipc.Client // loopback client for extension process + ipcClient atomic.Pointer[ipc.Client] // loopback client for extension process ipcBackend *backend.LocalBackend ipcMu sync.Mutex ipcOnce sync.Once @@ -76,9 +76,7 @@ func withCoreR[T any](fn func(c lanterncore.Core) (T, error)) (T, error) { // StartIPCServer (extension process), falling back to lanternCore's client // (main app process). func getClient() (*ipc.Client, error) { - ipcMu.Lock() - c := ipcClient - ipcMu.Unlock() + c := ipcClient.Load() if c != nil { return c, nil } @@ -302,12 +300,14 @@ func StartIPCServer(platform utils.PlatformInterface, opts *utils.Opts) error { return struct{}{}, fmt.Errorf("error creating backend for IPC server: %v", err) } be.Start() - ipcBackend = be - ipcServer = ipc.NewServer(be, !common.IsMobile()) - ipcClient = newLoopbackClient(be) - if err := ipcServer.Start(); err != nil { + server := ipc.NewServer(be, !common.IsMobile()) + if err := server.Start(); err != nil { + be.Close() return struct{}{}, err } + ipcBackend = be + ipcServer = server + ipcClient.Store(newLoopbackClient(be)) return struct{}{}, nil }) return err @@ -317,6 +317,7 @@ func CloseIPCServer() error { _, err := utils.RunOffCgoStack(func() (struct{}, error) { ipcMu.Lock() defer ipcMu.Unlock() + ipcClient.Store(nil) if ipcBackend != nil { ipcBackend.Close() ipcBackend = nil @@ -325,7 +326,6 @@ func CloseIPCServer() error { ipcServer.Close() ipcServer = nil } - ipcClient = nil return struct{}{}, nil }) return err diff --git a/lantern-core/mobile/mobile_test.go b/lantern-core/mobile/mobile_test.go index db752f92ae..62434bffea 100644 --- a/lantern-core/mobile/mobile_test.go +++ b/lantern-core/mobile/mobile_test.go @@ -1,5 +1,38 @@ package mobile +import ( + "testing" + "time" + + "github.com/getlantern/radiance/ipc" +) + +func TestGetClientDoesNotWaitForIPCLifecycleLock(t *testing.T) { + want := &ipc.Client{} + ipcClient.Store(want) + t.Cleanup(func() { + ipcClient.Store(nil) + }) + + ipcMu.Lock() + defer ipcMu.Unlock() + + result := make(chan *ipc.Client, 1) + go func() { + client, _ := getClient() + result <- client + }() + + select { + case got := <-result: + if got != want { + t.Fatalf("getClient() = %p, want %p", got, want) + } + case <-time.After(time.Second): + t.Fatal("getClient blocked on the IPC lifecycle lock") + } +} + // // todo implement a mock for all test cases // func radianceOptions() radiance.Options { // return radiance.Options{ diff --git a/macos/Runner/VPN/VPNBase.swift b/macos/Runner/VPN/VPNBase.swift index 4c43eff841..a4214b4a52 100644 --- a/macos/Runner/VPN/VPNBase.swift +++ b/macos/Runner/VPN/VPNBase.swift @@ -5,11 +5,65 @@ import NetworkExtension -enum VPNManagerError: Swift.Error { +enum VPNManagerError: LocalizedError { case userDisallowedVPNConfigurations case loadingProviderFailed case savingProviderFailed + case operationInProgress case unknown + + var errorDescription: String? { + switch self { + case .userDisallowedVPNConfigurations: + return "VPN configurations are not allowed." + case .loadingProviderFailed: + return "Unable to load the VPN configuration." + case .savingProviderFailed: + return "Unable to save the VPN configuration." + case .operationInProgress: + return "A VPN operation is already in progress." + case .unknown: + return "An unknown VPN error occurred." + } + } +} + +enum VPNConnectionAction: Equatable { + case startTunnel + case sendCommandToExtension +} + +func vpnConnectionAction(for status: NEVPNStatus) throws -> VPNConnectionAction { + switch status { + case .connected: + return .sendCommandToExtension + case .disconnected: + return .startTunnel + case .connecting, .disconnecting, .reasserting: + throw VPNManagerError.operationInProgress + case .invalid: + throw VPNManagerError.loadingProviderFailed + @unknown default: + throw VPNManagerError.unknown + } +} + +enum VPNStopAction: Equatable { + case stopTunnel + case alreadyStopped +} + +func vpnStopAction(for status: NEVPNStatus) throws -> VPNStopAction { + switch status { + case .connected, .connecting, .reasserting: + return .stopTunnel + case .disconnected, .disconnecting: + return .alreadyStopped + case .invalid: + throw VPNManagerError.loadingProviderFailed + @unknown default: + throw VPNManagerError.unknown + } } protocol VPNBase: ObservableObject { diff --git a/macos/Runner/VPN/VPNManager.swift b/macos/Runner/VPN/VPNManager.swift index d7f14ba972..8a60d57186 100644 --- a/macos/Runner/VPN/VPNManager.swift +++ b/macos/Runner/VPN/VPNManager.swift @@ -140,17 +140,13 @@ class VPNManager: VPNBase { let options = ["netEx.StartReason": NSString("Lantern")] appLogger.log("Calling manager.connection.startVPNTunnel..") - if manager.connection.status == .connected || manager.connection.status == .connecting { + switch try vpnConnectionAction(for: manager.connection.status) { + case .sendCommandToExtension: appLogger.info("VPN is already connected, sending command to extension") - do { - let result = try await triggerExtensionMethod( - methodName: "Lantern" - ) - return - } catch { - // Rethrow so caller can handle it - throw error - } + _ = try await triggerExtensionMethod(methodName: "Lantern") + return + case .startTunnel: + break } try self.manager.connection.startVPNTunnel(options: options) @@ -168,18 +164,16 @@ class VPNManager: VPNBase { "netEx.ServerName": serverName as NSString, ] - if manager.connection.status == .connected || manager.connection.status == .connecting { + switch try vpnConnectionAction(for: manager.connection.status) { + case .sendCommandToExtension: appLogger.info("VPN is already connected, sending command to extension") - do { - let result = try await triggerExtensionMethod( - methodName: "PrivateServer", - params: ["server": serverName] - ) - return - } catch { - // Rethrow so caller can handle it - throw error - } + _ = try await triggerExtensionMethod( + methodName: "PrivateServer", + params: ["server": serverName] + ) + return + case .startTunnel: + break } try self.manager.connection.startVPNTunnel(options: options) @@ -195,9 +189,13 @@ class VPNManager: VPNBase { func stopTunnel() async throws { appLogger.log("Stopping tunnel..") await syncStatus() - guard connectionStatus == .connected else { - appLogger.log("In unexpected state: \(connectionStatus)") + let status = manager.connection.status + switch try vpnStopAction(for: status) { + case .alreadyStopped: + appLogger.log("VPN is already stopped or stopping: \(status)") return + case .stopTunnel: + break } if manager.isOnDemandEnabled { diff --git a/macos/RunnerTests/RunnerTests.swift b/macos/RunnerTests/RunnerTests.swift index 69cf3b3491..b77d4fdb8a 100644 --- a/macos/RunnerTests/RunnerTests.swift +++ b/macos/RunnerTests/RunnerTests.swift @@ -1,10 +1,36 @@ -@testable import Lantern import Foundation +import NetworkExtension import SystemExtensions import XCTest +@testable import Lantern + final class RunnerTests: XCTestCase { + func testVPNConnectionActions() throws { + XCTAssertEqual(try vpnConnectionAction(for: .disconnected), .startTunnel) + XCTAssertEqual(try vpnConnectionAction(for: .connected), .sendCommandToExtension) + + for status in [NEVPNStatus.connecting, .disconnecting, .reasserting] { + XCTAssertThrowsError(try vpnConnectionAction(for: status)) { error in + guard let vpnError = error as? VPNManagerError, + case .operationInProgress = vpnError + else { + return XCTFail("Expected operationInProgress for \(status), got \(error)") + } + } + } + } + + func testVPNStopActions() throws { + for status in [NEVPNStatus.connected, .connecting, .reasserting] { + XCTAssertEqual(try vpnStopAction(for: status), .stopTunnel) + } + for status in [NEVPNStatus.disconnected, .disconnecting] { + XCTAssertEqual(try vpnStopAction(for: status), .alreadyStopped) + } + } + func testHashBundleIsStableForIdenticalContents() throws { let firstURL = try createExtensionBundle( name: "First.systemextension", From ff31932eb0f4b214a868d4c5ffc822b0c57a59e9 Mon Sep 17 00:00:00 2001 From: atavism Date: Mon, 3 Aug 2026 07:41:13 -0700 Subject: [PATCH 2/9] code review updates --- lantern-core/mobile/mobile.go | 73 +++++++++---- lantern-core/mobile/mobile_test.go | 85 ---------------- lantern-core/vpn_tunnel/vpn_tunnel.go | 33 +++--- lantern-core/vpn_tunnel/vpn_tunnel_test.go | 113 ++++++++++++++++++--- macos/Runner/VPN/VPNBase.swift | 22 ++-- macos/Runner/VPN/VPNManager.swift | 15 +-- macos/RunnerTests/RunnerTests.swift | 14 +-- 7 files changed, 188 insertions(+), 167 deletions(-) diff --git a/lantern-core/mobile/mobile.go b/lantern-core/mobile/mobile.go index f5ad4e6e55..b05e82edbe 100644 --- a/lantern-core/mobile/mobile.go +++ b/lantern-core/mobile/mobile.go @@ -27,14 +27,17 @@ import ( ) var ( - lanternCore atomic.Value - errLanternNotReady = errors.New("radiance not initialized") - - ipcServer *ipc.Server - ipcClient atomic.Pointer[ipc.Client] // loopback client for extension process - ipcBackend *backend.LocalBackend - ipcMu sync.Mutex - ipcOnce sync.Once + lanternCore atomic.Value + errLanternNotReady = errors.New("radiance not initialized") + errIPCStartCanceled = errors.New("IPC server startup canceled") + + ipcServer *ipc.Server + ipcClient atomic.Pointer[ipc.Client] // loopback client for extension process + ipcBackend *backend.LocalBackend + ipcMu sync.Mutex + ipcStarting bool + ipcClosing bool + ipcGeneration uint64 ) func getCore() (lanterncore.Core, error) { @@ -283,10 +286,23 @@ func StopVPN() error { func StartIPCServer(platform utils.PlatformInterface, opts *utils.Opts) error { _, err := utils.RunOffCgoStack(func() (struct{}, error) { ipcMu.Lock() - defer ipcMu.Unlock() if ipcServer != nil { + ipcMu.Unlock() return struct{}{}, nil } + if ipcStarting || ipcClosing { + ipcMu.Unlock() + return struct{}{}, errLanternNotReady + } + ipcStarting = true + generation := ipcGeneration + ipcMu.Unlock() + defer func() { + ipcMu.Lock() + ipcStarting = false + ipcMu.Unlock() + }() + // The backend's config fetcher captures common.GetBaseURL() at // construction, so the environment must be set before // NewLocalBackend — SetupRadiance's SetStagingEnv runs too late on @@ -313,9 +329,18 @@ func StartIPCServer(platform utils.PlatformInterface, opts *utils.Opts) error { be.Close() return struct{}{}, err } + + ipcMu.Lock() + if generation != ipcGeneration { + ipcMu.Unlock() + _ = server.Close() + be.Close() + return struct{}{}, errIPCStartCanceled + } ipcBackend = be ipcServer = server ipcClient.Store(newLoopbackClient(be)) + ipcMu.Unlock() return struct{}{}, nil }) return err @@ -324,15 +349,29 @@ func StartIPCServer(platform utils.PlatformInterface, opts *utils.Opts) error { func CloseIPCServer() error { _, err := utils.RunOffCgoStack(func() (struct{}, error) { ipcMu.Lock() - defer ipcMu.Unlock() - ipcClient.Store(nil) - if ipcBackend != nil { - ipcBackend.Close() - ipcBackend = nil + if ipcClosing { + ipcMu.Unlock() + return struct{}{}, nil } - if ipcServer != nil { - ipcServer.Close() - ipcServer = nil + ipcClosing = true + ipcGeneration++ + ipcClient.Store(nil) + be := ipcBackend + server := ipcServer + ipcBackend = nil + ipcServer = nil + ipcMu.Unlock() + defer func() { + ipcMu.Lock() + ipcClosing = false + ipcMu.Unlock() + }() + + if server != nil { + _ = server.Close() + } + if be != nil { + be.Close() } return struct{}{}, nil }) diff --git a/lantern-core/mobile/mobile_test.go b/lantern-core/mobile/mobile_test.go index 62434bffea..63949d7d0d 100644 --- a/lantern-core/mobile/mobile_test.go +++ b/lantern-core/mobile/mobile_test.go @@ -32,88 +32,3 @@ func TestGetClientDoesNotWaitForIPCLifecycleLock(t *testing.T) { t.Fatal("getClient blocked on the IPC lifecycle lock") } } - -// // todo implement a mock for all test cases -// func radianceOptions() radiance.Options { -// return radiance.Options{ -// DataDir: os.TempDir(), -// LogDir: os.TempDir(), -// DeviceID: "test-123", -// Locale: "en-us", -// } -// } - -// func TestSetupRadiance(t *testing.T) { -// rr, err := radiance.NewRadiance(radianceOptions()) -// assert.Nil(t, err) -// assert.NotNil(t, rr) - -// } - -// // // skip this test for now -// // func TestStartVPN(t *testing.T) { -// // data := radianceOptions().DataDir -// // log := radianceOptions().LogDir -// // rr, err := client.NewVPNClient(data, log, nil, false) -// // assert.Nil(t, err) -// // assert.NotNil(t, rr) -// // err1 := rr.StartVPN() -// // assert.Nil(t, err1) -// // } - -// func TestCreateUser(t *testing.T) { -// rr, err := radiance.NewRadiance(radianceOptions()) -// api := rr.APIHandler() -// assert.Nil(t, err) -// assert.NotNil(t, rr) -// user, err := api.NewUser(context.Background()) -// assert.Nil(t, err) -// assert.NotNil(t, user) -// } - -// func TestSubscriptionRedirect(t *testing.T) { -// rr, err := radiance.NewRadiance(radianceOptions()) -// apiClient := rr.APIHandler() -// assert.Nil(t, err) -// assert.NotNil(t, rr) -// data := api.PaymentRedirectData{ -// Provider: "stripe", -// Plan: "monthly", -// DeviceName: "test-123", -// Email: "test@getlantern.org", -// BillingType: api.SubscriptionTypeSubscription, -// } -// user, err := apiClient.SubscriptionPaymentRedirectURL(context.Background(), data) -// assert.Nil(t, err) -// assert.NotNil(t, user) -// } - -// func TestUserData(t *testing.T) { -// rr, err := radiance.NewRadiance(radianceOptions()) -// api := rr.APIHandler() -// assert.Nil(t, err) -// assert.NotNil(t, rr) -// user, err := api.UserData(context.Background()) -// assert.Nil(t, err) -// assert.NotNil(t, user) -// } - -// func TestPlans(t *testing.T) { -// rr, err := radiance.NewRadiance(radianceOptions()) -// api := rr.APIHandler() -// assert.Nil(t, err) -// assert.NotNil(t, rr) -// plans, err := api.SubscriptionPlans(context.Background(), "non-store") -// assert.Nil(t, err) -// assert.NotNil(t, plans) -// } - -// func TestOAuthLoginUrl(t *testing.T) { -// rr, err := radiance.NewRadiance(radianceOptions()) -// api := rr.APIHandler() -// assert.Nil(t, err) -// assert.NotNil(t, rr) -// user, err := api.OAuthLoginUrl(context.Background(), "google") -// assert.Nil(t, err) -// assert.NotNil(t, user) -// } diff --git a/lantern-core/vpn_tunnel/vpn_tunnel.go b/lantern-core/vpn_tunnel/vpn_tunnel.go index af32d1f799..70ec3ca5d0 100644 --- a/lantern-core/vpn_tunnel/vpn_tunnel.go +++ b/lantern-core/vpn_tunnel/vpn_tunnel.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "log/slog" + "sync" "github.com/getlantern/radiance/ipc" "github.com/getlantern/radiance/vpn" @@ -15,12 +16,15 @@ const ( InternalTagAutoAll InternalTag = "auto-all" ) -// StartVPN is the gomobile entry point for Mobile.StartVPN (Android -// MainActivity / iOS VPNManager). It is also reached from Jigar's -// onSmartLocation rewrite in server_selection.dart via startVPN(force: true) -// → lantern.startVPN() → Mobile.StartVPN, which expects "switch back to -// auto" to work on a live tunnel. Delegate to ConnectToServer so the -// VPNStatus → /server/selected dispatch handles that case. +var connectMu sync.Mutex + +type vpnClient interface { + VPNStatus(context.Context) (vpn.VPNStatus, error) + ConnectVPN(context.Context, string) error + SelectServer(context.Context, string) error +} + +// StartVPN starts the tunnel with automatic server selection. func StartVPN(ctx context.Context, client *ipc.Client) error { slog.Info("StartVPN called") return ConnectToServer(ctx, client, vpn.AutoSelectTag) @@ -30,16 +34,15 @@ func StopVPN(ctx context.Context, client *ipc.Client) error { return client.DisconnectVPN(ctx) } -// ConnectToServer switches the live tunnel to a specific server or, when the -// caller passes an empty tag or vpn.AutoSelectTag, back to auto-select. -// Radiance normalizes the empty-tag case server-side (fac9089) for both -// ConnectVPN and SelectServer. -// -// The caller is responsible for putting a deadline on ctx — the connect -// path involves real network work (DNS, TLS, sing-box bring-up) and we -// don't want a hung lanternd to stall the UI forever. LanternCore.ConnectVPN -// uses 60 s. +// ConnectToServer starts the tunnel or changes the selected server. func ConnectToServer(ctx context.Context, client *ipc.Client, tag string) error { + return connectToServer(ctx, client, tag) +} + +func connectToServer(ctx context.Context, client vpnClient, tag string) error { + connectMu.Lock() + defer connectMu.Unlock() + slog.Debug("Connecting to VPN server", "tag", tag) // Switch outbounds on the live tunnel when already connected; diff --git a/lantern-core/vpn_tunnel/vpn_tunnel_test.go b/lantern-core/vpn_tunnel/vpn_tunnel_test.go index e5cf11f800..2f87f6a039 100644 --- a/lantern-core/vpn_tunnel/vpn_tunnel_test.go +++ b/lantern-core/vpn_tunnel/vpn_tunnel_test.go @@ -1,17 +1,100 @@ package vpn_tunnel -// func TestStartVPN(t *testing.T) { -// radiance.NewRadiance(radianceOptions()) -// pltf := stub.NewPlatformInterfaceStub() -// err := StartVPN(pltf, &utils.Opts{}) -// assert.NoError(t, err) -// } - -// func radianceOptions() radiance.Options { -// return radiance.Options{ -// DataDir: os.TempDir(), -// LogDir: os.TempDir(), -// DeviceID: "test-123", -// Locale: "en-us", -// } -// } +import ( + "context" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/getlantern/radiance/vpn" +) + +type blockingClient struct { + statusCalls atomic.Int32 + connectCalls atomic.Int32 + statusCalled chan struct{} + release chan struct{} + releaseOnce sync.Once +} + +func (c *blockingClient) VPNStatus(ctx context.Context) (vpn.VPNStatus, error) { + call := c.statusCalls.Add(1) + c.statusCalled <- struct{}{} + if call == 1 { + select { + case <-c.release: + case <-ctx.Done(): + return vpn.Disconnected, ctx.Err() + } + } + return vpn.Disconnected, nil +} + +func (c *blockingClient) ConnectVPN(context.Context, string) error { + c.connectCalls.Add(1) + return nil +} + +func (c *blockingClient) SelectServer(context.Context, string) error { + return nil +} + +func (c *blockingClient) unblock() { + c.releaseOnce.Do(func() { close(c.release) }) +} + +func TestConnectToServerSerializesRequests(t *testing.T) { + client := &blockingClient{ + statusCalled: make(chan struct{}, 2), + release: make(chan struct{}), + } + t.Cleanup(client.unblock) + + firstDone := make(chan error, 1) + go func() { + firstDone <- connectToServer(context.Background(), client, "first") + }() + waitForStatusCall(t, client.statusCalled) + + secondDone := make(chan error, 1) + go func() { + secondDone <- connectToServer(context.Background(), client, "second") + }() + + select { + case <-client.statusCalled: + t.Fatal("second request entered while the first was still running") + case <-time.After(100 * time.Millisecond): + } + + client.unblock() + waitForResult(t, firstDone) + waitForStatusCall(t, client.statusCalled) + waitForResult(t, secondDone) + + if got := client.connectCalls.Load(); got != 2 { + t.Fatalf("ConnectVPN called %d times, want 2", got) + } +} + +func waitForStatusCall(t *testing.T, called <-chan struct{}) { + t.Helper() + select { + case <-called: + case <-time.After(time.Second): + t.Fatal("timed out waiting for VPNStatus") + } +} + +func waitForResult(t *testing.T, result <-chan error) { + t.Helper() + select { + case err := <-result: + if err != nil { + t.Fatal(err) + } + case <-time.After(time.Second): + t.Fatal("timed out waiting for connect request") + } +} diff --git a/macos/Runner/VPN/VPNBase.swift b/macos/Runner/VPN/VPNBase.swift index a4214b4a52..c938adfa9c 100644 --- a/macos/Runner/VPN/VPNBase.swift +++ b/macos/Runner/VPN/VPNBase.swift @@ -28,17 +28,12 @@ enum VPNManagerError: LocalizedError { } } -enum VPNConnectionAction: Equatable { - case startTunnel - case sendCommandToExtension -} - -func vpnConnectionAction(for status: NEVPNStatus) throws -> VPNConnectionAction { +func shouldStartNewTunnel(for status: NEVPNStatus) throws -> Bool { switch status { case .connected: - return .sendCommandToExtension + return false case .disconnected: - return .startTunnel + return true case .connecting, .disconnecting, .reasserting: throw VPNManagerError.operationInProgress case .invalid: @@ -48,17 +43,12 @@ func vpnConnectionAction(for status: NEVPNStatus) throws -> VPNConnectionAction } } -enum VPNStopAction: Equatable { - case stopTunnel - case alreadyStopped -} - -func vpnStopAction(for status: NEVPNStatus) throws -> VPNStopAction { +func shouldStopTunnel(for status: NEVPNStatus) throws -> Bool { switch status { case .connected, .connecting, .reasserting: - return .stopTunnel + return true case .disconnected, .disconnecting: - return .alreadyStopped + return false case .invalid: throw VPNManagerError.loadingProviderFailed @unknown default: diff --git a/macos/Runner/VPN/VPNManager.swift b/macos/Runner/VPN/VPNManager.swift index 8a60d57186..dbd18b0834 100644 --- a/macos/Runner/VPN/VPNManager.swift +++ b/macos/Runner/VPN/VPNManager.swift @@ -140,13 +140,10 @@ class VPNManager: VPNBase { let options = ["netEx.StartReason": NSString("Lantern")] appLogger.log("Calling manager.connection.startVPNTunnel..") - switch try vpnConnectionAction(for: manager.connection.status) { - case .sendCommandToExtension: + if try !shouldStartNewTunnel(for: manager.connection.status) { appLogger.info("VPN is already connected, sending command to extension") _ = try await triggerExtensionMethod(methodName: "Lantern") return - case .startTunnel: - break } try self.manager.connection.startVPNTunnel(options: options) @@ -164,16 +161,13 @@ class VPNManager: VPNBase { "netEx.ServerName": serverName as NSString, ] - switch try vpnConnectionAction(for: manager.connection.status) { - case .sendCommandToExtension: + if try !shouldStartNewTunnel(for: manager.connection.status) { appLogger.info("VPN is already connected, sending command to extension") _ = try await triggerExtensionMethod( methodName: "PrivateServer", params: ["server": serverName] ) return - case .startTunnel: - break } try self.manager.connection.startVPNTunnel(options: options) @@ -190,12 +184,9 @@ class VPNManager: VPNBase { appLogger.log("Stopping tunnel..") await syncStatus() let status = manager.connection.status - switch try vpnStopAction(for: status) { - case .alreadyStopped: + if try !shouldStopTunnel(for: status) { appLogger.log("VPN is already stopped or stopping: \(status)") return - case .stopTunnel: - break } if manager.isOnDemandEnabled { diff --git a/macos/RunnerTests/RunnerTests.swift b/macos/RunnerTests/RunnerTests.swift index b77d4fdb8a..231a5dada5 100644 --- a/macos/RunnerTests/RunnerTests.swift +++ b/macos/RunnerTests/RunnerTests.swift @@ -7,12 +7,12 @@ import XCTest final class RunnerTests: XCTestCase { - func testVPNConnectionActions() throws { - XCTAssertEqual(try vpnConnectionAction(for: .disconnected), .startTunnel) - XCTAssertEqual(try vpnConnectionAction(for: .connected), .sendCommandToExtension) + func testVPNStartStates() throws { + XCTAssertTrue(try shouldStartNewTunnel(for: .disconnected)) + XCTAssertFalse(try shouldStartNewTunnel(for: .connected)) for status in [NEVPNStatus.connecting, .disconnecting, .reasserting] { - XCTAssertThrowsError(try vpnConnectionAction(for: status)) { error in + XCTAssertThrowsError(try shouldStartNewTunnel(for: status)) { error in guard let vpnError = error as? VPNManagerError, case .operationInProgress = vpnError else { @@ -22,12 +22,12 @@ final class RunnerTests: XCTestCase { } } - func testVPNStopActions() throws { + func testVPNStopStates() throws { for status in [NEVPNStatus.connected, .connecting, .reasserting] { - XCTAssertEqual(try vpnStopAction(for: status), .stopTunnel) + XCTAssertTrue(try shouldStopTunnel(for: status)) } for status in [NEVPNStatus.disconnected, .disconnecting] { - XCTAssertEqual(try vpnStopAction(for: status), .alreadyStopped) + XCTAssertFalse(try shouldStopTunnel(for: status)) } } From 11e957df9ca5e50dfcc2c1432820e31711bd46ac Mon Sep 17 00:00:00 2001 From: atavism Date: Mon, 3 Aug 2026 11:06:43 -0700 Subject: [PATCH 3/9] code review updates --- lantern-core/mobile/mobile.go | 3 ++- lantern-core/mobile/mobile_test.go | 17 +++++++++++++ lantern-core/vpn_tunnel/vpn_tunnel.go | 11 +++++---- lantern-core/vpn_tunnel/vpn_tunnel_test.go | 26 +++++++++++++++----- macos/Runner/VPN/VPNBase.swift | 28 +++++++++++++++++++++- macos/Runner/VPN/VPNManager.swift | 10 ++++++++ macos/RunnerTests/RunnerTests.swift | 28 ++++++++++++++++++++++ 7 files changed, 111 insertions(+), 12 deletions(-) diff --git a/lantern-core/mobile/mobile.go b/lantern-core/mobile/mobile.go index b05e82edbe..124a7f777e 100644 --- a/lantern-core/mobile/mobile.go +++ b/lantern-core/mobile/mobile.go @@ -29,6 +29,7 @@ import ( var ( lanternCore atomic.Value errLanternNotReady = errors.New("radiance not initialized") + errIPCLifecycleBusy = errors.New("IPC server lifecycle operation in progress") errIPCStartCanceled = errors.New("IPC server startup canceled") ipcServer *ipc.Server @@ -292,7 +293,7 @@ func StartIPCServer(platform utils.PlatformInterface, opts *utils.Opts) error { } if ipcStarting || ipcClosing { ipcMu.Unlock() - return struct{}{}, errLanternNotReady + return struct{}{}, errIPCLifecycleBusy } ipcStarting = true generation := ipcGeneration diff --git a/lantern-core/mobile/mobile_test.go b/lantern-core/mobile/mobile_test.go index 63949d7d0d..afa6555025 100644 --- a/lantern-core/mobile/mobile_test.go +++ b/lantern-core/mobile/mobile_test.go @@ -1,6 +1,7 @@ package mobile import ( + "errors" "testing" "time" @@ -32,3 +33,19 @@ func TestGetClientDoesNotWaitForIPCLifecycleLock(t *testing.T) { t.Fatal("getClient blocked on the IPC lifecycle lock") } } + +func TestStartIPCServerReportsLifecycleBusy(t *testing.T) { + ipcMu.Lock() + ipcStarting = true + ipcMu.Unlock() + t.Cleanup(func() { + ipcMu.Lock() + ipcStarting = false + ipcMu.Unlock() + }) + + err := StartIPCServer(nil, nil) + if !errors.Is(err, errIPCLifecycleBusy) { + t.Fatalf("StartIPCServer() error = %v, want %v", err, errIPCLifecycleBusy) + } +} diff --git a/lantern-core/vpn_tunnel/vpn_tunnel.go b/lantern-core/vpn_tunnel/vpn_tunnel.go index 70ec3ca5d0..4c17c9c26f 100644 --- a/lantern-core/vpn_tunnel/vpn_tunnel.go +++ b/lantern-core/vpn_tunnel/vpn_tunnel.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "log/slog" - "sync" "github.com/getlantern/radiance/ipc" "github.com/getlantern/radiance/vpn" @@ -16,7 +15,7 @@ const ( InternalTagAutoAll InternalTag = "auto-all" ) -var connectMu sync.Mutex +var connectSem = make(chan struct{}, 1) type vpnClient interface { VPNStatus(context.Context) (vpn.VPNStatus, error) @@ -40,8 +39,12 @@ func ConnectToServer(ctx context.Context, client *ipc.Client, tag string) error } func connectToServer(ctx context.Context, client vpnClient, tag string) error { - connectMu.Lock() - defer connectMu.Unlock() + select { + case connectSem <- struct{}{}: + case <-ctx.Done(): + return ctx.Err() + } + defer func() { <-connectSem }() slog.Debug("Connecting to VPN server", "tag", tag) diff --git a/lantern-core/vpn_tunnel/vpn_tunnel_test.go b/lantern-core/vpn_tunnel/vpn_tunnel_test.go index 2f87f6a039..8b13eb4781 100644 --- a/lantern-core/vpn_tunnel/vpn_tunnel_test.go +++ b/lantern-core/vpn_tunnel/vpn_tunnel_test.go @@ -2,6 +2,7 @@ package vpn_tunnel import ( "context" + "errors" "sync" "sync/atomic" "testing" @@ -58,8 +59,10 @@ func TestConnectToServerSerializesRequests(t *testing.T) { waitForStatusCall(t, client.statusCalled) secondDone := make(chan error, 1) + secondCtx, cancelSecond := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancelSecond() go func() { - secondDone <- connectToServer(context.Background(), client, "second") + secondDone <- connectToServer(secondCtx, client, "second") }() select { @@ -68,11 +71,16 @@ func TestConnectToServerSerializesRequests(t *testing.T) { case <-time.After(100 * time.Millisecond): } + if err := waitForError(t, secondDone); !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("second request error = %v, want %v", err, context.DeadlineExceeded) + } + client.unblock() waitForResult(t, firstDone) - waitForStatusCall(t, client.statusCalled) - waitForResult(t, secondDone) + if err := connectToServer(context.Background(), client, "third"); err != nil { + t.Fatal(err) + } if got := client.connectCalls.Load(); got != 2 { t.Fatalf("ConnectVPN called %d times, want 2", got) } @@ -88,13 +96,19 @@ func waitForStatusCall(t *testing.T, called <-chan struct{}) { } func waitForResult(t *testing.T, result <-chan error) { + t.Helper() + if err := waitForError(t, result); err != nil { + t.Fatal(err) + } +} + +func waitForError(t *testing.T, result <-chan error) error { t.Helper() select { case err := <-result: - if err != nil { - t.Fatal(err) - } + return err case <-time.After(time.Second): t.Fatal("timed out waiting for connect request") + return nil } } diff --git a/macos/Runner/VPN/VPNBase.swift b/macos/Runner/VPN/VPNBase.swift index c938adfa9c..376f21e0d7 100644 --- a/macos/Runner/VPN/VPNBase.swift +++ b/macos/Runner/VPN/VPNBase.swift @@ -3,9 +3,10 @@ // Lantern // +import Foundation import NetworkExtension -enum VPNManagerError: LocalizedError { +enum VPNManagerError: LocalizedError, Equatable { case userDisallowedVPNConfigurations case loadingProviderFailed case savingProviderFailed @@ -28,6 +29,30 @@ enum VPNManagerError: LocalizedError { } } +/// Prevents two VPN lifecycle operations from running at the same time. +final class VPNOperationGate { + private let lock = NSLock() + private var active = false + + /// Claims the gate or reports that another operation is still running. + func begin() throws { + lock.lock() + defer { lock.unlock() } + guard !active else { + throw VPNManagerError.operationInProgress + } + active = true + } + + /// Releases the gate after the current operation finishes. + func end() { + lock.lock() + active = false + lock.unlock() + } +} + +/// Returns whether a new tunnel should start for the current system status. func shouldStartNewTunnel(for status: NEVPNStatus) throws -> Bool { switch status { case .connected: @@ -43,6 +68,7 @@ func shouldStartNewTunnel(for status: NEVPNStatus) throws -> Bool { } } +/// Returns whether the current tunnel should be stopped. func shouldStopTunnel(for status: NEVPNStatus) throws -> Bool { switch status { case .connected, .connecting, .reasserting: diff --git a/macos/Runner/VPN/VPNManager.swift b/macos/Runner/VPN/VPNManager.swift index dbd18b0834..8f02a6f028 100644 --- a/macos/Runner/VPN/VPNManager.swift +++ b/macos/Runner/VPN/VPNManager.swift @@ -9,6 +9,7 @@ import NetworkExtension class VPNManager: VPNBase { private var observer: NSObjectProtocol? + private let operationGate = VPNOperationGate() //Do not switch to NEVPNManager.shared() that is only for class app extension private var manager: NEVPNManager = NETunnelProviderManager() static let shared: VPNManager = VPNManager() @@ -135,6 +136,9 @@ class VPNManager: VPNBase { /// Starts the VPN tunnel. /// Loads VPN preferences and initiates the VPN connection. func startTunnel() async throws { + try operationGate.begin() + defer { operationGate.end() } + appLogger.log("Starting tunnel..") await self.setupVPN() let options = ["netEx.StartReason": NSString("Lantern")] @@ -154,6 +158,9 @@ class VPNManager: VPNBase { func connectToServer( serverName: String ) async throws { + try operationGate.begin() + defer { operationGate.end() } + await self.setupVPN() let options: [String: NSObject] = [ "netEx.Type": "PrivateServer" as NSString, @@ -181,6 +188,9 @@ class VPNManager: VPNBase { /// Stops the VPN tunnel. /// Terminates the VPN connection and updates the configuration. func stopTunnel() async throws { + try operationGate.begin() + defer { operationGate.end() } + appLogger.log("Stopping tunnel..") await syncStatus() let status = manager.connection.status diff --git a/macos/RunnerTests/RunnerTests.swift b/macos/RunnerTests/RunnerTests.swift index 231a5dada5..3df9f22cf9 100644 --- a/macos/RunnerTests/RunnerTests.swift +++ b/macos/RunnerTests/RunnerTests.swift @@ -7,6 +7,15 @@ import XCTest final class RunnerTests: XCTestCase { + private func assertVPNManagerError( + _ expected: VPNManagerError, + _ operation: () throws -> Void + ) { + XCTAssertThrowsError(try operation()) { error in + XCTAssertEqual(error as? VPNManagerError, expected) + } + } + func testVPNStartStates() throws { XCTAssertTrue(try shouldStartNewTunnel(for: .disconnected)) XCTAssertFalse(try shouldStartNewTunnel(for: .connected)) @@ -20,6 +29,10 @@ final class RunnerTests: XCTestCase { } } } + + assertVPNManagerError(.loadingProviderFailed) { + _ = try shouldStartNewTunnel(for: .invalid) + } } func testVPNStopStates() throws { @@ -29,6 +42,21 @@ final class RunnerTests: XCTestCase { for status in [NEVPNStatus.disconnected, .disconnecting] { XCTAssertFalse(try shouldStopTunnel(for: status)) } + + assertVPNManagerError(.loadingProviderFailed) { + _ = try shouldStopTunnel(for: .invalid) + } + } + + func testVPNOperationGateRejectsOverlap() throws { + let gate = VPNOperationGate() + try gate.begin() + assertVPNManagerError(.operationInProgress) { + try gate.begin() + } + gate.end() + XCTAssertNoThrow(try gate.begin()) + gate.end() } func testHashBundleIsStableForIdenticalContents() throws { From b0c443e4058fc106139b76618e7a871375b43e37 Mon Sep 17 00:00:00 2001 From: atavism Date: Mon, 3 Aug 2026 12:51:22 -0700 Subject: [PATCH 4/9] code review updates --- lantern-core/mobile/mobile_test.go | 9 ++++- macos/Runner/VPN/VPNBase.swift | 59 +++++++++++++++++++++------- macos/Runner/VPN/VPNManager.swift | 61 ++++++++++++++++++++++++----- macos/RunnerTests/RunnerTests.swift | 51 ++++++++++++++++++++---- 4 files changed, 147 insertions(+), 33 deletions(-) diff --git a/lantern-core/mobile/mobile_test.go b/lantern-core/mobile/mobile_test.go index afa6555025..fe040efe4e 100644 --- a/lantern-core/mobile/mobile_test.go +++ b/lantern-core/mobile/mobile_test.go @@ -36,11 +36,18 @@ func TestGetClientDoesNotWaitForIPCLifecycleLock(t *testing.T) { func TestStartIPCServerReportsLifecycleBusy(t *testing.T) { ipcMu.Lock() + previousServer := ipcServer + previousStarting := ipcStarting + previousClosing := ipcClosing + ipcServer = nil ipcStarting = true + ipcClosing = false ipcMu.Unlock() t.Cleanup(func() { ipcMu.Lock() - ipcStarting = false + ipcServer = previousServer + ipcStarting = previousStarting + ipcClosing = previousClosing ipcMu.Unlock() }) diff --git a/macos/Runner/VPN/VPNBase.swift b/macos/Runner/VPN/VPNBase.swift index 376f21e0d7..5fc1a41307 100644 --- a/macos/Runner/VPN/VPNBase.swift +++ b/macos/Runner/VPN/VPNBase.swift @@ -29,26 +29,55 @@ enum VPNManagerError: LocalizedError, Equatable { } } -/// Prevents two VPN lifecycle operations from running at the same time. -final class VPNOperationGate { - private let lock = NSLock() - private var active = false +/// Coordinates connection changes while allowing stop to cancel a pending start. +actor VPNLifecycleCoordinator { + private var nextConnectionID: UInt = 0 + private var activeConnectionID: UInt? + private var stopPending = false + private var stopWaiters: [CheckedContinuation] = [] - /// Claims the gate or reports that another operation is still running. - func begin() throws { - lock.lock() - defer { lock.unlock() } - guard !active else { + /// Starts a connection operation unless another lifecycle change owns the manager. + func beginConnectionOperation() throws -> UInt { + guard activeConnectionID == nil, !stopPending else { throw VPNManagerError.operationInProgress } - active = true + nextConnectionID &+= 1 + activeConnectionID = nextConnectionID + return nextConnectionID } - /// Releases the gate after the current operation finishes. - func end() { - lock.lock() - active = false - lock.unlock() + /// Returns false when a stop request has canceled this connection operation. + func canContinueConnectionOperation(_ id: UInt) -> Bool { + activeConnectionID == id && !stopPending + } + + /// Hands the manager to a waiting stop request after startup work has finished. + func endConnectionOperation(_ id: UInt) { + guard activeConnectionID == id else { return } + activeConnectionID = nil + let waiters = stopWaiters + stopWaiters.removeAll() + waiters.forEach { $0.resume() } + } + + /// Cancels any active connection operation and waits for its profile writes to finish. + /// The return value tells the caller to tear down even if the system status has not caught up. + func beginStopOperation() async throws -> Bool { + guard !stopPending else { + throw VPNManagerError.operationInProgress + } + stopPending = true + let canceledConnectionOperation = activeConnectionID != nil + guard canceledConnectionOperation else { return false } + await withCheckedContinuation { continuation in + stopWaiters.append(continuation) + } + return true + } + + /// Allows connection changes again once the stop request has completed. + func endStopOperation() { + stopPending = false } } diff --git a/macos/Runner/VPN/VPNManager.swift b/macos/Runner/VPN/VPNManager.swift index 8f02a6f028..fab0c95d29 100644 --- a/macos/Runner/VPN/VPNManager.swift +++ b/macos/Runner/VPN/VPNManager.swift @@ -9,7 +9,7 @@ import NetworkExtension class VPNManager: VPNBase { private var observer: NSObjectProtocol? - private let operationGate = VPNOperationGate() + private let lifecycleCoordinator = VPNLifecycleCoordinator() //Do not switch to NEVPNManager.shared() that is only for class app extension private var manager: NEVPNManager = NETunnelProviderManager() static let shared: VPNManager = VPNManager() @@ -136,11 +136,23 @@ class VPNManager: VPNBase { /// Starts the VPN tunnel. /// Loads VPN preferences and initiates the VPN connection. func startTunnel() async throws { - try operationGate.begin() - defer { operationGate.end() } + let operationID = try await lifecycleCoordinator.beginConnectionOperation() + do { + try await startTunnel(operationID: operationID) + } catch { + await lifecycleCoordinator.endConnectionOperation(operationID) + throw error + } + await lifecycleCoordinator.endConnectionOperation(operationID) + } + + private func startTunnel(operationID: UInt) async throws { appLogger.log("Starting tunnel..") await self.setupVPN() + guard await lifecycleCoordinator.canContinueConnectionOperation(operationID) else { + throw CancellationError() + } let options = ["netEx.StartReason": NSString("Lantern")] appLogger.log("Calling manager.connection.startVPNTunnel..") @@ -150,18 +162,33 @@ class VPNManager: VPNBase { return } - try self.manager.connection.startVPNTunnel(options: options) self.manager.isOnDemandEnabled = false try await self.saveThenLoadProvider() + guard await lifecycleCoordinator.canContinueConnectionOperation(operationID) else { + throw CancellationError() + } + try self.manager.connection.startVPNTunnel(options: options) } func connectToServer( serverName: String ) async throws { - try operationGate.begin() - defer { operationGate.end() } + let operationID = try await lifecycleCoordinator.beginConnectionOperation() + do { + try await connectToServer(serverName: serverName, operationID: operationID) + } catch { + await lifecycleCoordinator.endConnectionOperation(operationID) + throw error + } + await lifecycleCoordinator.endConnectionOperation(operationID) + } + + private func connectToServer(serverName: String, operationID: UInt) async throws { await self.setupVPN() + guard await lifecycleCoordinator.canContinueConnectionOperation(operationID) else { + throw CancellationError() + } let options: [String: NSObject] = [ "netEx.Type": "PrivateServer" as NSString, "netEx.StartReason": "Private server Initiated" as NSString, @@ -188,13 +215,29 @@ class VPNManager: VPNBase { /// Stops the VPN tunnel. /// Terminates the VPN connection and updates the configuration. func stopTunnel() async throws { - try operationGate.begin() - defer { operationGate.end() } + let canceledConnectionOperation = try await lifecycleCoordinator.beginStopOperation() + do { + try await stopTunnelAfterHandoff( + canceledConnectionOperation: canceledConnectionOperation) + } catch { + await lifecycleCoordinator.endStopOperation() + throw error + } + await lifecycleCoordinator.endStopOperation() + } + + private func stopTunnelAfterHandoff(canceledConnectionOperation: Bool) async throws { appLogger.log("Stopping tunnel..") await syncStatus() let status = manager.connection.status - if try !shouldStopTunnel(for: status) { + let shouldStop: Bool + if canceledConnectionOperation { + shouldStop = true + } else { + shouldStop = try shouldStopTunnel(for: status) + } + if !shouldStop { appLogger.log("VPN is already stopped or stopping: \(status)") return } diff --git a/macos/RunnerTests/RunnerTests.swift b/macos/RunnerTests/RunnerTests.swift index 3df9f22cf9..127332b45e 100644 --- a/macos/RunnerTests/RunnerTests.swift +++ b/macos/RunnerTests/RunnerTests.swift @@ -48,15 +48,50 @@ final class RunnerTests: XCTestCase { } } - func testVPNOperationGateRejectsOverlap() throws { - let gate = VPNOperationGate() - try gate.begin() - assertVPNManagerError(.operationInProgress) { - try gate.begin() + func testVPNLifecycleCoordinatorRejectsOverlappingConnections() async throws { + let coordinator = VPNLifecycleCoordinator() + let operationID = try await coordinator.beginConnectionOperation() + + do { + _ = try await coordinator.beginConnectionOperation() + XCTFail("Expected operationInProgress") + } catch { + XCTAssertEqual(error as? VPNManagerError, .operationInProgress) } - gate.end() - XCTAssertNoThrow(try gate.begin()) - gate.end() + + await coordinator.endConnectionOperation(operationID) + let nextOperationID = try await coordinator.beginConnectionOperation() + await coordinator.endConnectionOperation(nextOperationID) + } + + func testVPNStopCancelsStartupAndWaitsForHandoff() async throws { + let coordinator = VPNLifecycleCoordinator() + let operationID = try await coordinator.beginConnectionOperation() + let stopStarted = expectation(description: "Stop requested") + + let stopTask = Task { + stopStarted.fulfill() + return try await coordinator.beginStopOperation() + } + await fulfillment(of: [stopStarted]) + + while await coordinator.canContinueConnectionOperation(operationID) { + await Task.yield() + } + await coordinator.endConnectionOperation(operationID) + let canceledConnectionOperation = try await stopTask.value + XCTAssertTrue(canceledConnectionOperation) + + do { + _ = try await coordinator.beginConnectionOperation() + XCTFail("Expected operationInProgress while stop owns the manager") + } catch { + XCTAssertEqual(error as? VPNManagerError, .operationInProgress) + } + + await coordinator.endStopOperation() + let nextOperationID = try await coordinator.beginConnectionOperation() + await coordinator.endConnectionOperation(nextOperationID) } func testHashBundleIsStableForIdenticalContents() throws { From 9c63d1f5b0a26baf5ab6b0dca2db29e4c824f68b Mon Sep 17 00:00:00 2001 From: atavism Date: Mon, 3 Aug 2026 23:16:53 -0700 Subject: [PATCH 5/9] code review updates --- lantern-core/mobile/mobile_test.go | 4 +-- macos/Runner/VPN/VPNBase.swift | 56 +++++++++++++++++++++++------ macos/Runner/VPN/VPNManager.swift | 53 ++++++++++++++++----------- macos/RunnerTests/RunnerTests.swift | 39 +++++++++++++++----- 4 files changed, 111 insertions(+), 41 deletions(-) diff --git a/lantern-core/mobile/mobile_test.go b/lantern-core/mobile/mobile_test.go index fe040efe4e..214296a706 100644 --- a/lantern-core/mobile/mobile_test.go +++ b/lantern-core/mobile/mobile_test.go @@ -10,9 +10,9 @@ import ( func TestGetClientDoesNotWaitForIPCLifecycleLock(t *testing.T) { want := &ipc.Client{} - ipcClient.Store(want) + previousClient := ipcClient.Swap(want) t.Cleanup(func() { - ipcClient.Store(nil) + ipcClient.Store(previousClient) }) ipcMu.Lock() diff --git a/macos/Runner/VPN/VPNBase.swift b/macos/Runner/VPN/VPNBase.swift index 5fc1a41307..cc5d7110b4 100644 --- a/macos/Runner/VPN/VPNBase.swift +++ b/macos/Runner/VPN/VPNBase.swift @@ -31,10 +31,21 @@ enum VPNManagerError: LocalizedError, Equatable { /// Coordinates connection changes while allowing stop to cancel a pending start. actor VPNLifecycleCoordinator { + private struct StopWaiter { + let connectionID: UInt + let continuation: CheckedContinuation + } + private var nextConnectionID: UInt = 0 private var activeConnectionID: UInt? private var stopPending = false - private var stopWaiters: [CheckedContinuation] = [] + private var stopWaiter: StopWaiter? + private var stopHandoffTimeoutTask: Task? + private let stopHandoffTimeoutNanoseconds: UInt64 + + init(stopHandoffTimeoutNanoseconds: UInt64 = 5_000_000_000) { + self.stopHandoffTimeoutNanoseconds = stopHandoffTimeoutNanoseconds + } /// Starts a connection operation unless another lifecycle change owns the manager. func beginConnectionOperation() throws -> UInt { @@ -55,22 +66,29 @@ actor VPNLifecycleCoordinator { func endConnectionOperation(_ id: UInt) { guard activeConnectionID == id else { return } activeConnectionID = nil - let waiters = stopWaiters - stopWaiters.removeAll() - waiters.forEach { $0.resume() } + finishStopHandoff(for: id) } - /// Cancels any active connection operation and waits for its profile writes to finish. + /// Cancels any active connection operation and briefly waits for it to hand off the manager. /// The return value tells the caller to tear down even if the system status has not caught up. func beginStopOperation() async throws -> Bool { guard !stopPending else { throw VPNManagerError.operationInProgress } stopPending = true - let canceledConnectionOperation = activeConnectionID != nil - guard canceledConnectionOperation else { return false } + guard let connectionID = activeConnectionID else { return false } + let timeout = stopHandoffTimeoutNanoseconds + await withCheckedContinuation { continuation in - stopWaiters.append(continuation) + stopWaiter = StopWaiter(connectionID: connectionID, continuation: continuation) + stopHandoffTimeoutTask = Task { [weak self] in + do { + try await Task.sleep(nanoseconds: timeout) + } catch { + return + } + await self?.expireStopHandoff(for: connectionID) + } } return true } @@ -79,6 +97,24 @@ actor VPNLifecycleCoordinator { func endStopOperation() { stopPending = false } + + private func finishStopHandoff(for connectionID: UInt) { + guard let waiter = stopWaiter, waiter.connectionID == connectionID else { return } + stopWaiter = nil + stopHandoffTimeoutTask?.cancel() + stopHandoffTimeoutTask = nil + waiter.continuation.resume() + } + + private func expireStopHandoff(for connectionID: UInt) { + guard let waiter = stopWaiter, waiter.connectionID == connectionID else { return } + stopWaiter = nil + stopHandoffTimeoutTask = nil + if activeConnectionID == connectionID { + activeConnectionID = nil + } + waiter.continuation.resume() + } } /// Returns whether a new tunnel should start for the current system status. @@ -102,10 +138,8 @@ func shouldStopTunnel(for status: NEVPNStatus) throws -> Bool { switch status { case .connected, .connecting, .reasserting: return true - case .disconnected, .disconnecting: + case .disconnected, .disconnecting, .invalid: return false - case .invalid: - throw VPNManagerError.loadingProviderFailed @unknown default: throw VPNManagerError.unknown } diff --git a/macos/Runner/VPN/VPNManager.swift b/macos/Runner/VPN/VPNManager.swift index fab0c95d29..23fd864550 100644 --- a/macos/Runner/VPN/VPNManager.swift +++ b/macos/Runner/VPN/VPNManager.swift @@ -136,18 +136,12 @@ class VPNManager: VPNBase { /// Starts the VPN tunnel. /// Loads VPN preferences and initiates the VPN connection. func startTunnel() async throws { - let operationID = try await lifecycleCoordinator.beginConnectionOperation() - do { + try await withConnectionOperation { operationID in try await startTunnel(operationID: operationID) - } catch { - await lifecycleCoordinator.endConnectionOperation(operationID) - throw error } - await lifecycleCoordinator.endConnectionOperation(operationID) } private func startTunnel(operationID: UInt) async throws { - appLogger.log("Starting tunnel..") await self.setupVPN() guard await lifecycleCoordinator.canContinueConnectionOperation(operationID) else { @@ -173,18 +167,12 @@ class VPNManager: VPNBase { func connectToServer( serverName: String ) async throws { - let operationID = try await lifecycleCoordinator.beginConnectionOperation() - do { + try await withConnectionOperation { operationID in try await connectToServer(serverName: serverName, operationID: operationID) - } catch { - await lifecycleCoordinator.endConnectionOperation(operationID) - throw error } - await lifecycleCoordinator.endConnectionOperation(operationID) } private func connectToServer(serverName: String, operationID: UInt) async throws { - await self.setupVPN() guard await lifecycleCoordinator.canContinueConnectionOperation(operationID) else { throw CancellationError() @@ -204,12 +192,10 @@ class VPNManager: VPNBase { return } + guard await lifecycleCoordinator.canContinueConnectionOperation(operationID) else { + throw CancellationError() + } try self.manager.connection.startVPNTunnel(options: options) - /// Enable on-demand to allow automatic reconnections - /// if error it will stuck in infinite loop - // self.manager.isOnDemandEnabled = true - // try await self.saveThenLoadProvider() - } /// Stops the VPN tunnel. @@ -227,8 +213,21 @@ class VPNManager: VPNBase { } private func stopTunnelAfterHandoff(canceledConnectionOperation: Bool) async throws { - appLogger.log("Stopping tunnel..") + + // A canceled start already owns the current manager. Stop it before any + // preference call can delay teardown again. + if canceledConnectionOperation { + let shouldSaveOnDemandChange = manager.isOnDemandEnabled + manager.isOnDemandEnabled = false + manager.connection.stopVPNTunnel() + if shouldSaveOnDemandChange { + try await manager.saveToPreferences() + } + appLogger.log("Tunnel stopped.") + return + } + await syncStatus() let status = manager.connection.status let shouldStop: Bool @@ -251,6 +250,20 @@ class VPNManager: VPNBase { appLogger.log("Tunnel stopped.") } + private func withConnectionOperation( + _ operation: (UInt) async throws -> T + ) async throws -> T { + let operationID = try await lifecycleCoordinator.beginConnectionOperation() + do { + let result = try await operation(operationID) + await lifecycleCoordinator.endConnectionOperation(operationID) + return result + } catch { + await lifecycleCoordinator.endConnectionOperation(operationID) + throw error + } + } + /// Saves the current VPN configuration to preferences and reloads it. private func saveThenLoadProvider() async throws { try await self.manager.saveToPreferences() diff --git a/macos/RunnerTests/RunnerTests.swift b/macos/RunnerTests/RunnerTests.swift index 127332b45e..7f385821bc 100644 --- a/macos/RunnerTests/RunnerTests.swift +++ b/macos/RunnerTests/RunnerTests.swift @@ -39,13 +39,9 @@ final class RunnerTests: XCTestCase { for status in [NEVPNStatus.connected, .connecting, .reasserting] { XCTAssertTrue(try shouldStopTunnel(for: status)) } - for status in [NEVPNStatus.disconnected, .disconnecting] { + for status in [NEVPNStatus.disconnected, .disconnecting, .invalid] { XCTAssertFalse(try shouldStopTunnel(for: status)) } - - assertVPNManagerError(.loadingProviderFailed) { - _ = try shouldStopTunnel(for: .invalid) - } } func testVPNLifecycleCoordinatorRejectsOverlappingConnections() async throws { @@ -67,15 +63,20 @@ final class RunnerTests: XCTestCase { func testVPNStopCancelsStartupAndWaitsForHandoff() async throws { let coordinator = VPNLifecycleCoordinator() let operationID = try await coordinator.beginConnectionOperation() - let stopStarted = expectation(description: "Stop requested") let stopTask = Task { - stopStarted.fulfill() return try await coordinator.beginStopOperation() } - await fulfillment(of: [stopStarted]) + let deadline = Date().addingTimeInterval(1) while await coordinator.canContinueConnectionOperation(operationID) { + if Date() >= deadline { + await coordinator.endConnectionOperation(operationID) + _ = try await stopTask.value + await coordinator.endStopOperation() + XCTFail("Stop request did not take ownership of the manager") + return + } await Task.yield() } await coordinator.endConnectionOperation(operationID) @@ -94,6 +95,28 @@ final class RunnerTests: XCTestCase { await coordinator.endConnectionOperation(nextOperationID) } + func testVPNStopForcesHandoffAfterTimeout() async throws { + let coordinator = VPNLifecycleCoordinator(stopHandoffTimeoutNanoseconds: 10_000_000) + let operationID = try await coordinator.beginConnectionOperation() + let stopFinished = expectation(description: "Stop handoff finished") + + let stopTask = Task { + let result = try await coordinator.beginStopOperation() + stopFinished.fulfill() + return result + } + await fulfillment(of: [stopFinished], timeout: 1) + let canceledConnectionOperation = try await stopTask.value + XCTAssertTrue(canceledConnectionOperation) + let canContinue = await coordinator.canContinueConnectionOperation(operationID) + XCTAssertFalse(canContinue) + + await coordinator.endConnectionOperation(operationID) + await coordinator.endStopOperation() + let nextOperationID = try await coordinator.beginConnectionOperation() + await coordinator.endConnectionOperation(nextOperationID) + } + func testHashBundleIsStableForIdenticalContents() throws { let firstURL = try createExtensionBundle( name: "First.systemextension", From 46e59bb5e4fb5c9535042b8a4a4658696233b4e4 Mon Sep 17 00:00:00 2001 From: atavism Date: Mon, 10 Aug 2026 10:03:49 -0700 Subject: [PATCH 6/9] code review updates --- lantern-core/mobile/mobile.go | 71 +++++++++++++++++++++++++++---- macos/Runner/VPN/VPNManager.swift | 15 +++---- 2 files changed, 67 insertions(+), 19 deletions(-) diff --git a/lantern-core/mobile/mobile.go b/lantern-core/mobile/mobile.go index 124a7f777e..263757fe50 100644 --- a/lantern-core/mobile/mobile.go +++ b/lantern-core/mobile/mobile.go @@ -41,6 +41,8 @@ var ( ipcGeneration uint64 ) +const ipcStartTimeout = 60 * time.Second + func getCore() (lanterncore.Core, error) { v := lanternCore.Load() if v == nil { @@ -320,16 +322,67 @@ func StartIPCServer(platform utils.PlatformInterface, opts *utils.Opts) error { TelemetryConsent: opts.TelemetryConsent, PlatformInterface: platform, } - be, err := backend.NewLocalBackend(context.Background(), bopts) - if err != nil { - return struct{}{}, fmt.Errorf("error creating backend for IPC server: %v", err) - } - be.Start() - server := ipc.NewServer(be, !common.IsMobile()) - if err := server.Start(); err != nil { - be.Close() - return struct{}{}, err + + // NewLocalBackend performs synchronous disk and platform setup that does + // not consistently observe its context. Bound the complete construction + // and server-start path here so a stalled startup cannot leave + // ipcStarting set forever. LocalBackend retains its context for its full + // lifetime, so keep that context independent from this startup deadline. + type startResult struct { + backend *backend.LocalBackend + server *ipc.Server + err error } + startupCtx, cancelStartup := context.WithTimeout( + context.Background(), + ipcStartTimeout, + ) + defer cancelStartup() + resultCh := make(chan startResult) + go func() { + result := startResult{} + be, err := backend.NewLocalBackend(context.Background(), bopts) + if err != nil { + result.err = fmt.Errorf("error creating backend for IPC server: %w", err) + } else { + be.Start() + server := ipc.NewServer(be, !common.IsMobile()) + if err := server.Start(); err != nil { + be.Close() + result.err = err + } else { + result.backend = be + result.server = server + } + } + + select { + case resultCh <- result: + case <-startupCtx.Done(): + if result.server != nil { + _ = result.server.Close() + } + if result.backend != nil { + result.backend.Close() + } + } + }() + + var result startResult + select { + case result = <-resultCh: + if result.err != nil { + return struct{}{}, result.err + } + case <-startupCtx.Done(): + return struct{}{}, fmt.Errorf( + "IPC server startup exceeded %s: %w", + ipcStartTimeout, + startupCtx.Err(), + ) + } + be := result.backend + server := result.server ipcMu.Lock() if generation != ipcGeneration { diff --git a/macos/Runner/VPN/VPNManager.swift b/macos/Runner/VPN/VPNManager.swift index 23fd864550..40c2628b94 100644 --- a/macos/Runner/VPN/VPNManager.swift +++ b/macos/Runner/VPN/VPNManager.swift @@ -145,7 +145,7 @@ class VPNManager: VPNBase { appLogger.log("Starting tunnel..") await self.setupVPN() guard await lifecycleCoordinator.canContinueConnectionOperation(operationID) else { - throw CancellationError() + throw VPNManagerError.operationInProgress } let options = ["netEx.StartReason": NSString("Lantern")] appLogger.log("Calling manager.connection.startVPNTunnel..") @@ -159,7 +159,7 @@ class VPNManager: VPNBase { self.manager.isOnDemandEnabled = false try await self.saveThenLoadProvider() guard await lifecycleCoordinator.canContinueConnectionOperation(operationID) else { - throw CancellationError() + throw VPNManagerError.operationInProgress } try self.manager.connection.startVPNTunnel(options: options) } @@ -175,7 +175,7 @@ class VPNManager: VPNBase { private func connectToServer(serverName: String, operationID: UInt) async throws { await self.setupVPN() guard await lifecycleCoordinator.canContinueConnectionOperation(operationID) else { - throw CancellationError() + throw VPNManagerError.operationInProgress } let options: [String: NSObject] = [ "netEx.Type": "PrivateServer" as NSString, @@ -193,7 +193,7 @@ class VPNManager: VPNBase { } guard await lifecycleCoordinator.canContinueConnectionOperation(operationID) else { - throw CancellationError() + throw VPNManagerError.operationInProgress } try self.manager.connection.startVPNTunnel(options: options) } @@ -230,12 +230,7 @@ class VPNManager: VPNBase { await syncStatus() let status = manager.connection.status - let shouldStop: Bool - if canceledConnectionOperation { - shouldStop = true - } else { - shouldStop = try shouldStopTunnel(for: status) - } + let shouldStop = try shouldStopTunnel(for: status) if !shouldStop { appLogger.log("VPN is already stopped or stopping: \(status)") return From bb6f524510d1897442e62fa622fb24ec43f75d5a Mon Sep 17 00:00:00 2001 From: atavism Date: Mon, 10 Aug 2026 11:26:48 -0700 Subject: [PATCH 7/9] code review updates --- macos/Runner/VPN/VPNBase.swift | 13 +++++++++ macos/Runner/VPN/VPNManager.swift | 45 +++++++++++++++++++---------- macos/RunnerTests/RunnerTests.swift | 8 +++++ 3 files changed, 51 insertions(+), 15 deletions(-) diff --git a/macos/Runner/VPN/VPNBase.swift b/macos/Runner/VPN/VPNBase.swift index cc5d7110b4..e8166b4dda 100644 --- a/macos/Runner/VPN/VPNBase.swift +++ b/macos/Runner/VPN/VPNBase.swift @@ -62,6 +62,19 @@ actor VPNLifecycleCoordinator { activeConnectionID == id && !stopPending } + /// Performs the final synchronous connection transition while this actor + /// still owns the lifecycle decision, so a stop cannot interleave between + /// the cancellation check and starting the system tunnel. + func performFinalConnectionTransition( + _ id: UInt, + _ transition: () throws -> T + ) throws -> T { + guard activeConnectionID == id, !stopPending else { + throw VPNManagerError.operationInProgress + } + return try transition() + } + /// Hands the manager to a waiting stop request after startup work has finished. func endConnectionOperation(_ id: UInt) { guard activeConnectionID == id else { return } diff --git a/macos/Runner/VPN/VPNManager.swift b/macos/Runner/VPN/VPNManager.swift index 40c2628b94..1fff09905e 100644 --- a/macos/Runner/VPN/VPNManager.swift +++ b/macos/Runner/VPN/VPNManager.swift @@ -158,10 +158,11 @@ class VPNManager: VPNBase { self.manager.isOnDemandEnabled = false try await self.saveThenLoadProvider() - guard await lifecycleCoordinator.canContinueConnectionOperation(operationID) else { - throw VPNManagerError.operationInProgress - } - try self.manager.connection.startVPNTunnel(options: options) + try await startOrNotifyExistingTunnel( + operationID: operationID, + options: options, + methodName: "Lantern" + ) } func connectToServer( @@ -183,19 +184,33 @@ class VPNManager: VPNBase { "netEx.ServerName": serverName as NSString, ] - if try !shouldStartNewTunnel(for: manager.connection.status) { - appLogger.info("VPN is already connected, sending command to extension") - _ = try await triggerExtensionMethod( - methodName: "PrivateServer", - params: ["server": serverName] - ) - return - } + try await startOrNotifyExistingTunnel( + operationID: operationID, + options: options, + methodName: "PrivateServer", + params: ["server": serverName] + ) + } - guard await lifecycleCoordinator.canContinueConnectionOperation(operationID) else { - throw VPNManagerError.operationInProgress + private func startOrNotifyExistingTunnel( + operationID: UInt, + options: [String: NSObject], + methodName: String, + params: [String: Any] = [:] + ) async throws { + let startedNewTunnel = try await lifecycleCoordinator.performFinalConnectionTransition( + operationID + ) { + if try !shouldStartNewTunnel(for: self.manager.connection.status) { + return false + } + try self.manager.connection.startVPNTunnel(options: options) + return true + } + if !startedNewTunnel { + appLogger.info("VPN is already connected, sending command to extension") + _ = try await triggerExtensionMethod(methodName: methodName, params: params) } - try self.manager.connection.startVPNTunnel(options: options) } /// Stops the VPN tunnel. diff --git a/macos/RunnerTests/RunnerTests.swift b/macos/RunnerTests/RunnerTests.swift index 7f385821bc..27a4530c41 100644 --- a/macos/RunnerTests/RunnerTests.swift +++ b/macos/RunnerTests/RunnerTests.swift @@ -79,6 +79,14 @@ final class RunnerTests: XCTestCase { } await Task.yield() } + do { + try await coordinator.performFinalConnectionTransition(operationID) { + XCTFail("Canceled connection transition must not run") + } + XCTFail("Expected operationInProgress for the canceled transition") + } catch { + XCTAssertEqual(error as? VPNManagerError, .operationInProgress) + } await coordinator.endConnectionOperation(operationID) let canceledConnectionOperation = try await stopTask.value XCTAssertTrue(canceledConnectionOperation) From f99d212f4de8df40328ea452fb32dc201d47fced Mon Sep 17 00:00:00 2001 From: atavism Date: Mon, 10 Aug 2026 16:01:08 -0700 Subject: [PATCH 8/9] code review updates --- lantern-core/mobile/ipc_lifecycle.go | 228 +++++++++++++++++++++++++++ lantern-core/mobile/mobile.go | 181 +-------------------- lantern-core/mobile/mobile_test.go | 30 ++-- 3 files changed, 245 insertions(+), 194 deletions(-) create mode 100644 lantern-core/mobile/ipc_lifecycle.go diff --git a/lantern-core/mobile/ipc_lifecycle.go b/lantern-core/mobile/ipc_lifecycle.go new file mode 100644 index 0000000000..3faf397d17 --- /dev/null +++ b/lantern-core/mobile/ipc_lifecycle.go @@ -0,0 +1,228 @@ +package mobile + +import ( + "context" + "errors" + "fmt" + "sync" + "sync/atomic" + "time" + + "github.com/getlantern/radiance/backend" + "github.com/getlantern/radiance/common" + "github.com/getlantern/radiance/common/env" + "github.com/getlantern/radiance/ipc" + + "github.com/getlantern/lantern/lantern-core/utils" +) + +const ipcStartTimeout = 60 * time.Second + +var ( + errIPCLifecycleBusy = errors.New("ipc server lifecycle operation in progress") + errIPCStartCanceled = errors.New("ipc server startup canceled") + + ipcClient atomic.Pointer[ipc.Client] // loopback client for extension process + ipcLifecycle ipcLifecycleState +) + +type ipcLifecycleState struct { + mu sync.Mutex + server *ipc.Server + backend *backend.LocalBackend + starting bool + closing bool + generation uint64 +} + +type ipcResources struct { + server *ipc.Server + backend *backend.LocalBackend +} + +func (resources ipcResources) close() { + if resources.server != nil { + _ = resources.server.Close() + } + if resources.backend != nil { + resources.backend.Close() + } +} + +type ipcStartResult struct { + resources ipcResources + err error +} + +// getClient returns an IPC client. It prefers the loopback client created by +// StartIPCServer (extension process), falling back to lanternCore's client +// (main app process). +func getClient() (*ipc.Client, error) { + if client := ipcClient.Load(); client != nil { + return client, nil + } + core, err := getCore() + if err != nil { + return nil, err + } + return core.Client(), nil +} + +func StartIPCServer(platform utils.PlatformInterface, opts *utils.Opts) error { + _, err := utils.RunOffCgoStack(func() (struct{}, error) { + return struct{}{}, startIPCServer(platform, opts) + }) + return err +} + +func startIPCServer(platform utils.PlatformInterface, opts *utils.Opts) error { + generation, shouldStart, err := beginIPCStart() + if err != nil || !shouldStart { + return err + } + defer finishIPCStart() + + // The backend's config fetcher captures common.GetBaseURL() at + // construction, so the environment must be set before NewLocalBackend. + // SetupRadiance's SetStagingEnv runs too late on Android, where + // StartIPCServer is called first. + if opts.IsStaging() { + env.SetStagingEnv() + } + + startupCtx, cancelStartup := context.WithTimeout(context.Background(), ipcStartTimeout) + defer cancelStartup() + + resources, err := startIPCResources(startupCtx, backend.Options{ + DataDir: opts.DataDir, + LogDir: opts.LogDir, + Locale: opts.Locale, + LogLevel: opts.LogLevel, + DeviceID: opts.Deviceid, + TelemetryConsent: opts.TelemetryConsent, + PlatformInterface: platform, + }) + if err != nil { + return err + } + return publishIPCResources(generation, resources) +} + +func beginIPCStart() (generation uint64, shouldStart bool, err error) { + ipcLifecycle.mu.Lock() + defer ipcLifecycle.mu.Unlock() + + if ipcLifecycle.server != nil { + return 0, false, nil + } + if ipcLifecycle.starting || ipcLifecycle.closing { + return 0, false, errIPCLifecycleBusy + } + ipcLifecycle.starting = true + return ipcLifecycle.generation, true, nil +} + +func finishIPCStart() { + ipcLifecycle.mu.Lock() + ipcLifecycle.starting = false + ipcLifecycle.mu.Unlock() +} + +// startIPCResources bounds backend construction and server startup even though +// NewLocalBackend does not consistently observe a context. LocalBackend retains +// its context for its full lifetime, so construction uses an independent +// background context while startupCtx controls waiting and late-result cleanup. +func startIPCResources(startupCtx context.Context, opts backend.Options) (ipcResources, error) { + resultCh := make(chan ipcStartResult) + go func() { + result := newIPCResources(opts) + select { + case resultCh <- result: + case <-startupCtx.Done(): + result.resources.close() + } + }() + + select { + case result := <-resultCh: + return result.resources, result.err + case <-startupCtx.Done(): + return ipcResources{}, fmt.Errorf( + "ipc server startup exceeded %s: %w", + ipcStartTimeout, + startupCtx.Err(), + ) + } +} + +func newIPCResources(opts backend.Options) ipcStartResult { + localBackend, err := backend.NewLocalBackend(context.Background(), opts) + if err != nil { + return ipcStartResult{err: fmt.Errorf("error creating backend for IPC server: %w", err)} + } + localBackend.Start() + + server := ipc.NewServer(localBackend, !common.IsMobile()) + if err := server.Start(); err != nil { + localBackend.Close() + return ipcStartResult{err: err} + } + return ipcStartResult{resources: ipcResources{ + server: server, + backend: localBackend, + }} +} + +func publishIPCResources(generation uint64, resources ipcResources) error { + ipcLifecycle.mu.Lock() + if generation != ipcLifecycle.generation { + ipcLifecycle.mu.Unlock() + resources.close() + return errIPCStartCanceled + } + ipcLifecycle.backend = resources.backend + ipcLifecycle.server = resources.server + ipcClient.Store(newLoopbackClient(resources.backend)) + ipcLifecycle.mu.Unlock() + return nil +} + +func CloseIPCServer() error { + _, err := utils.RunOffCgoStack(func() (struct{}, error) { + resources, shouldClose := beginIPCClose() + if !shouldClose { + return struct{}{}, nil + } + defer finishIPCClose() + + resources.close() + return struct{}{}, nil + }) + return err +} + +func beginIPCClose() (ipcResources, bool) { + ipcLifecycle.mu.Lock() + defer ipcLifecycle.mu.Unlock() + + if ipcLifecycle.closing { + return ipcResources{}, false + } + ipcLifecycle.closing = true + ipcLifecycle.generation++ + ipcClient.Store(nil) + + resources := ipcResources{ + server: ipcLifecycle.server, + backend: ipcLifecycle.backend, + } + ipcLifecycle.server = nil + ipcLifecycle.backend = nil + return resources, true +} + +func finishIPCClose() { + ipcLifecycle.mu.Lock() + ipcLifecycle.closing = false + ipcLifecycle.mu.Unlock() +} diff --git a/lantern-core/mobile/mobile.go b/lantern-core/mobile/mobile.go index 263757fe50..f49f7d9a5a 100644 --- a/lantern-core/mobile/mobile.go +++ b/lantern-core/mobile/mobile.go @@ -7,18 +7,14 @@ import ( "fmt" "log/slog" "os" - "sync" "sync/atomic" "time" _ "golang.org/x/mobile/bind" "github.com/getlantern/radiance/account" - "github.com/getlantern/radiance/backend" "github.com/getlantern/radiance/common" - "github.com/getlantern/radiance/common/env" "github.com/getlantern/radiance/common/settings" - "github.com/getlantern/radiance/ipc" lanterncore "github.com/getlantern/lantern/lantern-core" "github.com/getlantern/lantern/lantern-core/logs" @@ -27,22 +23,10 @@ import ( ) var ( - lanternCore atomic.Value - errLanternNotReady = errors.New("radiance not initialized") - errIPCLifecycleBusy = errors.New("IPC server lifecycle operation in progress") - errIPCStartCanceled = errors.New("IPC server startup canceled") - - ipcServer *ipc.Server - ipcClient atomic.Pointer[ipc.Client] // loopback client for extension process - ipcBackend *backend.LocalBackend - ipcMu sync.Mutex - ipcStarting bool - ipcClosing bool - ipcGeneration uint64 + lanternCore atomic.Value + errLanternNotReady = errors.New("radiance not initialized") ) -const ipcStartTimeout = 60 * time.Second - func getCore() (lanterncore.Core, error) { v := lanternCore.Load() if v == nil { @@ -79,21 +63,6 @@ func withCoreR[T any](fn func(c lanterncore.Core) (T, error)) (T, error) { }) } -// getClient returns an IPC client. It prefers the loopback client created by -// StartIPCServer (extension process), falling back to lanternCore's client -// (main app process). -func getClient() (*ipc.Client, error) { - c := ipcClient.Load() - if c != nil { - return c, nil - } - core, err := getCore() - if err != nil { - return nil, err - } - return core.Client(), nil -} - // SetQAEnvOverrides applies QA-only environment overrides before Radiance starts. func SetQAEnvOverrides(outboundSocks, tz string) error { if outboundSocks != "" { @@ -286,152 +255,6 @@ func StopVPN() error { return err } -func StartIPCServer(platform utils.PlatformInterface, opts *utils.Opts) error { - _, err := utils.RunOffCgoStack(func() (struct{}, error) { - ipcMu.Lock() - if ipcServer != nil { - ipcMu.Unlock() - return struct{}{}, nil - } - if ipcStarting || ipcClosing { - ipcMu.Unlock() - return struct{}{}, errIPCLifecycleBusy - } - ipcStarting = true - generation := ipcGeneration - ipcMu.Unlock() - defer func() { - ipcMu.Lock() - ipcStarting = false - ipcMu.Unlock() - }() - - // The backend's config fetcher captures common.GetBaseURL() at - // construction, so the environment must be set before - // NewLocalBackend — SetupRadiance's SetStagingEnv runs too late on - // Android, where StartIPCServer is called first. - if opts.IsStaging() { - env.SetStagingEnv() - } - bopts := backend.Options{ - DataDir: opts.DataDir, - LogDir: opts.LogDir, - Locale: opts.Locale, - LogLevel: opts.LogLevel, - DeviceID: opts.Deviceid, - TelemetryConsent: opts.TelemetryConsent, - PlatformInterface: platform, - } - - // NewLocalBackend performs synchronous disk and platform setup that does - // not consistently observe its context. Bound the complete construction - // and server-start path here so a stalled startup cannot leave - // ipcStarting set forever. LocalBackend retains its context for its full - // lifetime, so keep that context independent from this startup deadline. - type startResult struct { - backend *backend.LocalBackend - server *ipc.Server - err error - } - startupCtx, cancelStartup := context.WithTimeout( - context.Background(), - ipcStartTimeout, - ) - defer cancelStartup() - resultCh := make(chan startResult) - go func() { - result := startResult{} - be, err := backend.NewLocalBackend(context.Background(), bopts) - if err != nil { - result.err = fmt.Errorf("error creating backend for IPC server: %w", err) - } else { - be.Start() - server := ipc.NewServer(be, !common.IsMobile()) - if err := server.Start(); err != nil { - be.Close() - result.err = err - } else { - result.backend = be - result.server = server - } - } - - select { - case resultCh <- result: - case <-startupCtx.Done(): - if result.server != nil { - _ = result.server.Close() - } - if result.backend != nil { - result.backend.Close() - } - } - }() - - var result startResult - select { - case result = <-resultCh: - if result.err != nil { - return struct{}{}, result.err - } - case <-startupCtx.Done(): - return struct{}{}, fmt.Errorf( - "IPC server startup exceeded %s: %w", - ipcStartTimeout, - startupCtx.Err(), - ) - } - be := result.backend - server := result.server - - ipcMu.Lock() - if generation != ipcGeneration { - ipcMu.Unlock() - _ = server.Close() - be.Close() - return struct{}{}, errIPCStartCanceled - } - ipcBackend = be - ipcServer = server - ipcClient.Store(newLoopbackClient(be)) - ipcMu.Unlock() - return struct{}{}, nil - }) - return err -} - -func CloseIPCServer() error { - _, err := utils.RunOffCgoStack(func() (struct{}, error) { - ipcMu.Lock() - if ipcClosing { - ipcMu.Unlock() - return struct{}{}, nil - } - ipcClosing = true - ipcGeneration++ - ipcClient.Store(nil) - be := ipcBackend - server := ipcServer - ipcBackend = nil - ipcServer = nil - ipcMu.Unlock() - defer func() { - ipcMu.Lock() - ipcClosing = false - ipcMu.Unlock() - }() - - if server != nil { - _ = server.Close() - } - if be != nil { - be.Close() - } - return struct{}{}, nil - }) - return err -} - // IsTagAvailable checks if a server with the given tag exists in the server list. // Returns true if the tag is found. Returns true when the check cannot be performed // (fail-open: allows connection attempts to proceed normally). diff --git a/lantern-core/mobile/mobile_test.go b/lantern-core/mobile/mobile_test.go index 214296a706..9c39fe57cd 100644 --- a/lantern-core/mobile/mobile_test.go +++ b/lantern-core/mobile/mobile_test.go @@ -15,8 +15,8 @@ func TestGetClientDoesNotWaitForIPCLifecycleLock(t *testing.T) { ipcClient.Store(previousClient) }) - ipcMu.Lock() - defer ipcMu.Unlock() + ipcLifecycle.mu.Lock() + defer ipcLifecycle.mu.Unlock() result := make(chan *ipc.Client, 1) go func() { @@ -35,20 +35,20 @@ func TestGetClientDoesNotWaitForIPCLifecycleLock(t *testing.T) { } func TestStartIPCServerReportsLifecycleBusy(t *testing.T) { - ipcMu.Lock() - previousServer := ipcServer - previousStarting := ipcStarting - previousClosing := ipcClosing - ipcServer = nil - ipcStarting = true - ipcClosing = false - ipcMu.Unlock() + ipcLifecycle.mu.Lock() + previousServer := ipcLifecycle.server + previousStarting := ipcLifecycle.starting + previousClosing := ipcLifecycle.closing + ipcLifecycle.server = nil + ipcLifecycle.starting = true + ipcLifecycle.closing = false + ipcLifecycle.mu.Unlock() t.Cleanup(func() { - ipcMu.Lock() - ipcServer = previousServer - ipcStarting = previousStarting - ipcClosing = previousClosing - ipcMu.Unlock() + ipcLifecycle.mu.Lock() + ipcLifecycle.server = previousServer + ipcLifecycle.starting = previousStarting + ipcLifecycle.closing = previousClosing + ipcLifecycle.mu.Unlock() }) err := StartIPCServer(nil, nil) From 05a329d5f09fe243821b7ea653a9d49b6a5a334b Mon Sep 17 00:00:00 2001 From: atavism Date: Mon, 10 Aug 2026 16:04:36 -0700 Subject: [PATCH 9/9] code review updates --- lantern-core/mobile/ipc_lifecycle.go | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/lantern-core/mobile/ipc_lifecycle.go b/lantern-core/mobile/ipc_lifecycle.go index 3faf397d17..27707c308d 100644 --- a/lantern-core/mobile/ipc_lifecycle.go +++ b/lantern-core/mobile/ipc_lifecycle.go @@ -40,6 +40,7 @@ type ipcResources struct { backend *backend.LocalBackend } +// close shuts down the server and its backend. func (resources ipcResources) close() { if resources.server != nil { _ = resources.server.Close() @@ -68,6 +69,8 @@ func getClient() (*ipc.Client, error) { return core.Client(), nil } +// StartIPCServer starts the local IPC server. Once it is running, repeated +// calls are no-ops. func StartIPCServer(platform utils.PlatformInterface, opts *utils.Opts) error { _, err := utils.RunOffCgoStack(func() (struct{}, error) { return struct{}{}, startIPCServer(platform, opts) @@ -75,6 +78,8 @@ func StartIPCServer(platform utils.PlatformInterface, opts *utils.Opts) error { return err } +// startIPCServer builds the backend, then publishes it if shutdown did not +// happen in the meantime. func startIPCServer(platform utils.PlatformInterface, opts *utils.Opts) error { generation, shouldStart, err := beginIPCStart() if err != nil || !shouldStart { @@ -108,6 +113,8 @@ func startIPCServer(platform utils.PlatformInterface, opts *utils.Opts) error { return publishIPCResources(generation, resources) } +// beginIPCStart reserves the lifecycle for one startup attempt. The generation +// tells us whether shutdown happened before that attempt finished. func beginIPCStart() (generation uint64, shouldStart bool, err error) { ipcLifecycle.mu.Lock() defer ipcLifecycle.mu.Unlock() @@ -122,16 +129,15 @@ func beginIPCStart() (generation uint64, shouldStart bool, err error) { return ipcLifecycle.generation, true, nil } +// finishIPCStart releases the startup guard. func finishIPCStart() { ipcLifecycle.mu.Lock() ipcLifecycle.starting = false ipcLifecycle.mu.Unlock() } -// startIPCResources bounds backend construction and server startup even though -// NewLocalBackend does not consistently observe a context. LocalBackend retains -// its context for its full lifetime, so construction uses an independent -// background context while startupCtx controls waiting and late-result cleanup. +// startIPCResources waits for backend setup and cleans up if the result arrives +// after the caller has stopped waiting. func startIPCResources(startupCtx context.Context, opts backend.Options) (ipcResources, error) { resultCh := make(chan ipcStartResult) go func() { @@ -155,6 +161,7 @@ func startIPCResources(startupCtx context.Context, opts backend.Options) (ipcRes } } +// newIPCResources builds the local backend and starts its IPC server. func newIPCResources(opts backend.Options) ipcStartResult { localBackend, err := backend.NewLocalBackend(context.Background(), opts) if err != nil { @@ -173,6 +180,8 @@ func newIPCResources(opts backend.Options) ipcStartResult { }} } +// publishIPCResources makes a completed startup visible to clients. If +// CloseIPCServer ran during setup, it closes the new resources instead. func publishIPCResources(generation uint64, resources ipcResources) error { ipcLifecycle.mu.Lock() if generation != ipcLifecycle.generation { @@ -187,6 +196,7 @@ func publishIPCResources(generation uint64, resources ipcResources) error { return nil } +// CloseIPCServer detaches the current IPC resources before shutting them down. func CloseIPCServer() error { _, err := utils.RunOffCgoStack(func() (struct{}, error) { resources, shouldClose := beginIPCClose() @@ -201,6 +211,7 @@ func CloseIPCServer() error { return err } +// beginIPCClose detaches the live resources and invalidates any in-flight start. func beginIPCClose() (ipcResources, bool) { ipcLifecycle.mu.Lock() defer ipcLifecycle.mu.Unlock() @@ -221,6 +232,7 @@ func beginIPCClose() (ipcResources, bool) { return resources, true } +// finishIPCClose releases the shutdown guard. func finishIPCClose() { ipcLifecycle.mu.Lock() ipcLifecycle.closing = false