[PR #6998] [client] Publish engine route manager only after successful start #29394

Open
opened 2026-08-05 08:07:59 -04:00 by saavagebueno · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/netbirdio/netbird/pull/6998
Author: @Optic00
Created: 7/30/2026
Status: 🔄 Open

Base: mainHead: codex/fix-ios-route-manager-race


📝 Commits (2)

  • 4b3ae3b fix iOS route manager initialization
  • c2239d5 synchronize route manager publication

📊 Changes

5 files changed (+122 additions, -4 deletions)

View changed files

📝 client/internal/engine.go (+23 -2)
📝 client/internal/engine_test.go (+56 -0)
📝 client/ios/NetBirdSDK/client.go (+8 -2)
client/ios/NetBirdSDK/route_manager.go (+17 -0)
client/ios/NetBirdSDK/route_manager_test.go (+18 -0)

📄 Description

Describe your changes

iOS SDK route calls read Engine.GetRouteManager while Engine.Start could still be assigning the field, so callers could observe a nil, partially initialized, or concurrently written route manager (a data race).

The engine now publishes the route manager through an atomic.Pointer only after Start completes successfully and unpublishes it at the top of teardown (shared by Stop and Start's failure path), so GetRouteManager returns either nil or a fully initialized manager.

The iOS SelectRoute/DeselectRoute bridges now return a "could not get route manager" error instead of panicking when the engine is not started; all other existing callers already nil-check.

Added unit tests for publish/unpublish/read ordering plus a concurrent publication test, run under go test -race, and an iOS helper test. Verified with the focused core tests under -race, the iOS helper test, an iOS arm64 CGO compile, and git diff --check.

N/A

Stack

Standalone PR based on main.

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)
  • This change does not modify the public API, gRPC protocols, functionality behavior, CLI / service flags, or introduce a new feature — OR I have discussed it with the NetBird team beforehand (link the issue / Slack thread in the description). See CONTRIBUTING.md.

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

  • I added/updated documentation for this change
  • Documentation is not needed for this change (explain why)

Internal concurrency fix; no user-facing behavior, API, or configuration changes.

Docs PR URL (required if "docs added" is checked)

N/A

Summary by CodeRabbit

  • Bug Fixes
    • Route management is now available only after successful startup and is cleared during shutdown.
    • Route selection and removal report a clear error when route management is unavailable.
    • Improved safety for concurrent route-manager access.
  • Tests
    • Added coverage for route-manager availability throughout the startup and shutdown lifecycle, including concurrent access.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/netbirdio/netbird/pull/6998 **Author:** [@Optic00](https://github.com/Optic00) **Created:** 7/30/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `codex/fix-ios-route-manager-race` --- ### 📝 Commits (2) - [`4b3ae3b`](https://github.com/netbirdio/netbird/commit/4b3ae3b5318e254b2439a3b961ab1921e54529a3) fix iOS route manager initialization - [`c2239d5`](https://github.com/netbirdio/netbird/commit/c2239d5238e3a9c33a5386020602c679fd6c9b9c) synchronize route manager publication ### 📊 Changes **5 files changed** (+122 additions, -4 deletions) <details> <summary>View changed files</summary> 📝 `client/internal/engine.go` (+23 -2) 📝 `client/internal/engine_test.go` (+56 -0) 📝 `client/ios/NetBirdSDK/client.go` (+8 -2) ➕ `client/ios/NetBirdSDK/route_manager.go` (+17 -0) ➕ `client/ios/NetBirdSDK/route_manager_test.go` (+18 -0) </details> ### 📄 Description ## Describe your changes iOS SDK route calls read `Engine.GetRouteManager` while `Engine.Start` could still be assigning the field, so callers could observe a nil, partially initialized, or concurrently written route manager (a data race). The engine now publishes the route manager through an `atomic.Pointer` only after `Start` completes successfully and unpublishes it at the top of teardown (shared by `Stop` and `Start`'s failure path), so `GetRouteManager` returns either nil or a fully initialized manager. The iOS `SelectRoute`/`DeselectRoute` bridges now return a "could not get route manager" error instead of panicking when the engine is not started; all other existing callers already nil-check. Added unit tests for publish/unpublish/read ordering plus a concurrent publication test, run under `go test -race`, and an iOS helper test. Verified with the focused core tests under `-race`, the iOS helper test, an iOS arm64 CGO compile, and `git diff --check`. ## Issue ticket number and link N/A ## Stack <!-- branch-stack --> Standalone PR based on `main`. ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [x] Created tests that fail without the change (if possible) - [x] This change does **not** modify the public API, gRPC protocols, functionality behavior, CLI / service flags, or introduce a new feature — **OR** I have discussed it with the NetBird team beforehand (link the issue / Slack thread in the description). See [CONTRIBUTING.md](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTING.md#discuss-changes-with-the-netbird-team-first). > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) Internal concurrency fix; no user-facing behavior, API, or configuration changes. ### Docs PR URL (required if "docs added" is checked) N/A <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Route management is now available only after successful startup and is cleared during shutdown. * Route selection and removal report a clear error when route management is unavailable. * Improved safety for concurrent route-manager access. * **Tests** * Added coverage for route-manager availability throughout the startup and shutdown lifecycle, including concurrent access. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
saavagebueno added the pull-request label 2026-08-05 08:07:59 -04:00
Sign in to join this conversation.
No Label pull-request
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DYNR/netbird#29394