prevent PacketTunnel from wedging during cold startup - #8946
Open
atavism wants to merge 8 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR coordinates IPC startup and shutdown, serializes VPN connection requests, and adds explicit macOS tunnel state decisions with cancellation and concurrency tests. ChangesVPN lifecycle reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant VPNManager
participant VPNLifecycleCoordinator
participant MobileIPC
participant VPNTunnel
VPNManager->>VPNLifecycleCoordinator: acquire start or stop operation
VPNManager->>MobileIPC: start or connect through IPC
MobileIPC->>VPNTunnel: serialize connection request
VPNLifecycleCoordinator-->>VPNManager: cancel or reject conflicting operation
VPNManager->>VPNLifecycleCoordinator: complete operation
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent the macOS/iOS PacketTunnel (and related mobile IPC plumbing) from wedging during cold startup by making VPN lifecycle operations more deterministic and adding concurrency safeguards and tests.
Changes:
- Adds explicit state gating for VPN start/stop based on
NEVPNStatus, with a newoperationInProgresserror surfaced to callers. - Serializes core “connect/select server” requests in
lantern-core/vpn_tunnelto avoid overlapping tunnel operations. - Refactors mobile IPC client access to avoid blocking on lifecycle locks, and adds targeted concurrency tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| macos/RunnerTests/RunnerTests.swift | Adds unit tests validating new VPN start/stop state gating behavior. |
| macos/Runner/VPN/VPNManager.swift | Uses shared helpers to decide whether to start/stop vs. message the extension, improving state handling. |
| macos/Runner/VPN/VPNBase.swift | Introduces operationInProgress error + helper functions to map NEVPNStatus to actions/errors. |
| lantern-core/vpn_tunnel/vpn_tunnel.go | Adds request serialization around connect/select operations to prevent overlapping calls. |
| lantern-core/vpn_tunnel/vpn_tunnel_test.go | Adds a concurrency test asserting connect requests are serialized. |
| lantern-core/mobile/mobile.go | Makes IPC client access atomic and adds lifecycle guards/cancelation behavior for start/close races. |
| lantern-core/mobile/mobile_test.go | Adds a regression test ensuring getClient() doesn’t block on the IPC lifecycle mutex. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lantern-core/mobile/mobile.go (1)
289-343: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBound the IPC start path
StartIPCServersetsipcStartingbeforebackend.NewLocalBackend,be.Start(), andserver.Start(), but the backend construction does not consume the passed context, socontext.Background()provides no deadline. Startipc.StartIPCServerwith a context deadline as inStartVPN/StopVPNso a wedged backend startup cannot leaveipcStartingforever and block later calls witherrLanternNotReady.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lantern-core/mobile/mobile.go` around lines 289 - 343, Update StartIPCServer to use a bounded context with a deadline, matching the established StartVPN/StopVPN pattern, and pass it to backend.NewLocalBackend instead of context.Background(). Ensure the timeout covers backend construction and startup so ipcStarting cannot remain set indefinitely while preserving the existing cleanup and cancellation behavior.
🧹 Nitpick comments (1)
macos/RunnerTests/RunnerTests.swift (1)
10-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the
.invalidbranch.
testVPNStartStatesandtestVPNStopStatesdo not cover.invalid, which bothshouldStartNewTunnelandshouldStopTunnelmap toVPNManagerError.loadingProviderFailed. Add assertions for.invalidto lock in this error-mapping behavior for a critical-path decision helper.✅ Proposed additional assertions
for status in [NEVPNStatus.connecting, .disconnecting, .reasserting] { XCTAssertThrowsError(try shouldStartNewTunnel(for: status)) { error in guard let vpnError = error as? VPNManagerError, case .operationInProgress = vpnError else { return XCTFail("Expected operationInProgress for \(status), got \(error)") } } } + + XCTAssertThrowsError(try shouldStartNewTunnel(for: .invalid)) { error in + guard let vpnError = error as? VPNManagerError, + case .loadingProviderFailed = vpnError + else { + return XCTFail("Expected loadingProviderFailed for .invalid, got \(error)") + } + } } func testVPNStopStates() throws { for status in [NEVPNStatus.connected, .connecting, .reasserting] { XCTAssertTrue(try shouldStopTunnel(for: status)) } for status in [NEVPNStatus.disconnected, .disconnecting] { XCTAssertFalse(try shouldStopTunnel(for: status)) } + + XCTAssertThrowsError(try shouldStopTunnel(for: .invalid)) { error in + guard let vpnError = error as? VPNManagerError, + case .loadingProviderFailed = vpnError + else { + return XCTFail("Expected loadingProviderFailed for .invalid, got \(error)") + } + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@macos/RunnerTests/RunnerTests.swift` around lines 10 - 33, Add `.invalid` coverage to both `testVPNStartStates` and `testVPNStopStates`, asserting that `shouldStartNewTunnel(for:)` and `shouldStopTunnel(for:)` throw `VPNManagerError.loadingProviderFailed`. Keep the existing status assertions unchanged and verify the specific error case rather than only checking that an error occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lantern-core/vpn_tunnel/vpn_tunnel.go`:
- Around line 42-45: Replace the plain connectMu mutex acquisition in
connectToServer with a context-aware semaphore wait that can return when ctx is
canceled or reaches its deadline. Preserve serialized access to the VPN status,
server selection, and connection operations, and release the semaphore after the
existing deferred critical-section cleanup.
In `@macos/Runner/VPN/VPNManager.swift`:
- Around line 143-146: Serialize the shared VPNManager operations invoked by
MethodHandler: update startVPN, connectToServer, and stopVPN so their
independent Tasks cannot concurrently call startTunnel(), connectToServer(), or
stopTunnel(). Add a lock, serial queue, or actor/@MainActor isolation around
these entry points, preserving the existing already-connected
triggerExtensionMethod behavior while preventing overlapping tunnel operations.
---
Outside diff comments:
In `@lantern-core/mobile/mobile.go`:
- Around line 289-343: Update StartIPCServer to use a bounded context with a
deadline, matching the established StartVPN/StopVPN pattern, and pass it to
backend.NewLocalBackend instead of context.Background(). Ensure the timeout
covers backend construction and startup so ipcStarting cannot remain set
indefinitely while preserving the existing cleanup and cancellation behavior.
---
Nitpick comments:
In `@macos/RunnerTests/RunnerTests.swift`:
- Around line 10-33: Add `.invalid` coverage to both `testVPNStartStates` and
`testVPNStopStates`, asserting that `shouldStartNewTunnel(for:)` and
`shouldStopTunnel(for:)` throw `VPNManagerError.loadingProviderFailed`. Keep the
existing status assertions unchanged and verify the specific error case rather
than only checking that an error occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f11171f-288b-416e-9bb7-41745f6e5b99
📒 Files selected for processing (7)
lantern-core/mobile/mobile.golantern-core/mobile/mobile_test.golantern-core/vpn_tunnel/vpn_tunnel.golantern-core/vpn_tunnel/vpn_tunnel_test.gomacos/Runner/VPN/VPNBase.swiftmacos/Runner/VPN/VPNManager.swiftmacos/RunnerTests/RunnerTests.swift
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lantern-core/mobile/mobile_test.go`:
- Around line 38-47: Update the test setup around StartIPCServer to save all
lifecycle fields, explicitly set ipcServer to nil, ipcStarting to true, and
ipcClosing to false before asserting errIPCLifecycleBusy, then restore each
saved value in t.Cleanup instead of forcing ipcStarting to false.
In `@macos/Runner/VPN/VPNManager.swift`:
- Line 12: Update the VPNManager start/stop lifecycle around operationGate,
startTunnel(), and stopTunnel() so a stop request can cancel an in-progress
startup after startVPNTunnel() while preserving serialized profile persistence
through saveThenLoadProvider(). Add a cancellation-capable transition that
releases or coordinates the gate safely, lets stopTunnel() reach the .connecting
teardown path and invoke stopVPNTunnel(), and prevents races with startup
completion. Add a test that pauses startup after startVPNTunnel() and verifies
stopTunnel() sends the teardown request.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d518c4e-d5e5-4af5-8128-d007084147c0
📒 Files selected for processing (7)
lantern-core/mobile/mobile.golantern-core/mobile/mobile_test.golantern-core/vpn_tunnel/vpn_tunnel.golantern-core/vpn_tunnel/vpn_tunnel_test.gomacos/Runner/VPN/VPNBase.swiftmacos/Runner/VPN/VPNManager.swiftmacos/RunnerTests/RunnerTests.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- lantern-core/vpn_tunnel/vpn_tunnel.go
- lantern-core/mobile/mobile.go
- lantern-core/vpn_tunnel/vpn_tunnel_test.go
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
macos/Runner/VPN/VPNManager.swift:200
- stopTunnel() now throws for NEVPNStatus.invalid (via shouldStopTunnel) after syncStatus() returns early when no VPN profile exists. This can surface a user-visible STOP_FAILED even though there’s simply nothing to stop (e.g., first launch/no profile). Consider treating .invalid as a no-op in stopTunnel (or explicitly detecting the "no configured profile" case) to keep stop idempotent.
let status = manager.connection.status
if try !shouldStopTunnel(for: status) {
appLogger.log("VPN is already stopped or stopping: \(status)")
return
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
macos/Runner/VPN/VPNManager.swift:207
connectToServer(serverName:operationID:)checkscanContinueConnectionOperationright aftersetupVPN(), but a stop request can cancel the operation after that guard and beforestartVPNTunnel(...)is invoked. In that case we can still start a new tunnel even thoughbeginStopOperation()is pending, which undermines the intended handoff/cancellation behavior. Add a secondcanContinueConnectionOperationguard immediately before starting the tunnel (mirroringstartTunnel(operationID:)).
try self.manager.connection.startVPNTunnel(options: options)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
macos/Runner/VPN/VPNManager.swift (1)
138-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the connection-operation wrapper.
startTunnel()andconnectToServer(serverName:)repeat the same begin/do/catch/end sequence. Each copy must callendConnectionOperationon both the success path and the failure path. A missed call leaves a stop request suspended inbeginStopOperation. One helper removes that risk for future entry points.♻️ Proposed helper
+ private func withConnectionOperation<T>( + _ body: (UInt) async throws -> T + ) async throws -> T { + let operationID = try await lifecycleCoordinator.beginConnectionOperation() + defer { Task { await lifecycleCoordinator.endConnectionOperation(operationID) } } + return try await body(operationID) + } + func startTunnel() async throws { - let operationID = try await lifecycleCoordinator.beginConnectionOperation() - do { - try await startTunnel(operationID: operationID) - } catch { - await lifecycleCoordinator.endConnectionOperation(operationID) - throw error - } - await lifecycleCoordinator.endConnectionOperation(operationID) + try await withConnectionOperation { operationID in + try await self.startTunnel(operationID: operationID) + } }Note:
defercannot await, so the sketch spawns a detached release. If you prefer a deterministic release, keep the explicit do/catch inside the single helper instead.Also applies to: 173-184
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@macos/Runner/VPN/VPNManager.swift` around lines 138 - 147, Extract the duplicated begin/do/catch/end lifecycle sequence from startTunnel() and connectToServer(serverName:) into one async helper, ensuring endConnectionOperation is awaited on both success and failure paths before returning or rethrowing. Update both entry points to use this helper so future connection operations share the same deterministic cleanup.macos/RunnerTests/RunnerTests.swift (1)
70-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBound the spin loop and drop the misleading expectation.
Line 73 fulfills
stopStartedbefore the task callsbeginStopOperation(). The expectation therefore does not prove that the stop request setstopPending. The loop at Lines 78-80 provides the real synchronization, becausecanContinueConnectionOperationreturns false only afterstopPendingbecomes true.The loop has no deadline. If the coordinator regresses and never sets
stopPending, this test hangs the test run instead of failing. Add a deadline.💚 Proposed bounded wait
- 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) { + let deadline = Date().addingTimeInterval(5) + while await coordinator.canContinueConnectionOperation(operationID) { + if Date() > deadline { + stopTask.cancel() + return XCTFail("Stop request did not take ownership of the manager") + } await Task.yield() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@macos/RunnerTests/RunnerTests.swift` around lines 70 - 80, Remove the misleading stopStarted expectation and its fulfillment from the stopTask setup, since it occurs before beginStopOperation. Bound the synchronization loop around coordinator.canContinueConnectionOperation(operationID) with a deadline or timeout so the test fails instead of hanging if stopPending is never set, while preserving the existing Task.yield behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@macos/Runner/VPN/VPNBase.swift`:
- Around line 65-76: Update beginStopOperation to wait for the active connection
handoff only up to a defined deadline, then resume the stop flow and force
teardown when the deadline expires. Coordinate the timeout with the existing
stopWaiters/endConnectionOperation wakeup path so each continuation is resumed
exactly once, while preserving the current operationInProgress guard and
immediate false result when no connection is active.
---
Nitpick comments:
In `@macos/Runner/VPN/VPNManager.swift`:
- Around line 138-147: Extract the duplicated begin/do/catch/end lifecycle
sequence from startTunnel() and connectToServer(serverName:) into one async
helper, ensuring endConnectionOperation is awaited on both success and failure
paths before returning or rethrowing. Update both entry points to use this
helper so future connection operations share the same deterministic cleanup.
In `@macos/RunnerTests/RunnerTests.swift`:
- Around line 70-80: Remove the misleading stopStarted expectation and its
fulfillment from the stopTask setup, since it occurs before beginStopOperation.
Bound the synchronization loop around
coordinator.canContinueConnectionOperation(operationID) with a deadline or
timeout so the test fails instead of hanging if stopPending is never set, while
preserving the existing Task.yield behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d8d81a63-49ca-47ca-97f7-8c359d4e1c35
📒 Files selected for processing (4)
lantern-core/mobile/mobile_test.gomacos/Runner/VPN/VPNBase.swiftmacos/Runner/VPN/VPNManager.swiftmacos/RunnerTests/RunnerTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- lantern-core/mobile/mobile_test.go
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
macos/Runner/VPN/VPNManager.swift:238
- The
canceledConnectionOperationbranch here is redundant/unreachable: the function returns earlier whencanceledConnectionOperationis true, so this conditional adds dead code and makes the stop decision harder to follow. You can simplify to a singleshouldStopTunnel(for:)call.
if canceledConnectionOperation {
shouldStop = true
} else {
shouldStop = try shouldStopTunnel(for: status)
}
Contributor
|
@atavism Is this ready for review? |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (5)
macos/Runner/VPN/VPNManager.swift:197
- Same as above: throwing CancellationError here makes client-visible errors unclear. Prefer a VPNManagerError so callers get a stable, localizedDescription-backed message.
guard await lifecycleCoordinator.canContinueConnectionOperation(operationID) else {
throw CancellationError()
}
macos/Runner/VPN/VPNManager.swift:179
- Same as above: this CancellationError will be shown to users via error.localizedDescription (CONNECT_TO_SERVER_FAILED). Use VPNManagerError.operationInProgress (or another LocalizedError) for a clearer message.
guard await lifecycleCoordinator.canContinueConnectionOperation(operationID) else {
throw CancellationError()
}
macos/Runner/VPN/VPNManager.swift:163
- Same as above: throwing CancellationError after save/load can surface a confusing error to callers. Prefer throwing VPNManagerError.operationInProgress so UI gets a clear, user-facing message.
guard await lifecycleCoordinator.canContinueConnectionOperation(operationID) else {
throw CancellationError()
}
macos/Runner/VPN/VPNManager.swift:149
- Throwing CancellationError here can leak an unhelpful/opaque localizedDescription up to the Flutter layer (MethodHandler wraps it as START_FAILED). Since this path really means a stop operation has taken ownership, return a VPNManagerError with a clear message instead.
This issue also appears in the following locations of the same file:
- line 161
- line 177
- line 195
guard await lifecycleCoordinator.canContinueConnectionOperation(operationID) else {
throw CancellationError()
}
macos/Runner/VPN/VPNManager.swift:238
- This branch is dead code: stopTunnelAfterHandoff returns early when canceledConnectionOperation is true, so the if/else here will always take the else path. Simplify to avoid misleading control flow.
let shouldStop: Bool
if canceledConnectionOperation {
shouldStop = true
} else {
shouldStop = try shouldStopTunnel(for: status)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves https://github.com/getlantern/engineering/issues/3747
Summary by CodeRabbit
Bug Fixes
Tests