[client] Phase 3.5 + 3.7d-h of #5989: network-change ICE recovery + GUI mode tab (stack 2/4)

[MichaelUray]

[client] Phase 3.5 + 3.7d-h of #5989: network-change ICE recovery + GUI mode tab

Branch: pr/b-phase3.5-network-change → base pr/a-p2p-dynamic-foundation (stacked PR — depends on PR-A landing first)
Compare: https://github.com/netbirdio/netbird/compare/MichaelUray:netbird:pr/a-p2p-dynamic-foundation...MichaelUray:netbird:pr/b-phase3.5-network-change?expand=1

---

Summary

This PR adds network-change handling, signal-trigger ICE re-attach, a short-grace post-network-change ICE-failure delay (so we don't burn the exponential backoff on the well-known "first ICE pair-check after WiFi/cellular handover fails on stale NAT mappings"), the Guard-Loop fix from the closed PR #5805, and the GUI surface for Connection Mode + timeouts in the client UI.

This is the second of four stacked PRs implementing #5989. It builds on PR-A.

Why

The Phase-1+2+3 foundation (PR-A) handles steady-state operation well, but real devices do not stay in steady state:

• Laptops roam between WiFi and cellular.
• Mobile devices wake from Doze/StandBy with stale NAT mappings.
• LTE modems briefly drop and re-attach.

After such an event the existing ICE agent is bound to a departing local interface and pion's pair-checks fail. The exponential backoff (PR-A) then kicks in and the peer stays on relay for minutes.

Two related fixes are needed:

1. Re-attach ICE after a network change — recreate the ICE worker on a fresh socket, send a new offer. (Phases 3.5, 3.7d, 3.7e.)
2. Short retry delay during the post-network-change grace window — the first pair-check often fails on stale mappings even when subsequent ones succeed. Falling back to the normal 1-minute initial backoff after that single failure leaves the peer on relay far longer than the underlying connectivity warrants. A 60-second grace window with 2-second retries lets ~4-5 attempts run while the new mapping primes. (Phases 3.7f, 3.7h.)

Plus: previously closed PR #5805 ("Fix ICE reconnection loop and buffer candidates before agent initialization") had a known reconnect-loop where ICE worker rebuild on every signal-state change racked up offers faster than pion could complete pair-checks. The clean fix is to skip new offers while the ICE agent is still in Connecting.

What's in this PR

Phase 3.5 — Reset on network change

• client/peer/conn: reset ICE backoff + recreate workerICE when the signal/relay layer reconnects after a network change.
• client/peer/conn: refactor onNetworkChange to use the in-place agent recreate path; drop SendOffer from onNetworkChange to fix offer-storm; revert and replace an interim worker_ice candidate-buffering attempt with a clean approach.

Phase 3.7d — Signal-trigger re-attach

• Re-attach ICE on every signal trigger so a peer that lost ICE while the daemon was Disconnected from signaling resumes correctly when signaling is back.

Phase 3.7e — Listener inside onNetworkChange

• Re-attach the ICE listener inside onNetworkChange itself so a network change fires a fresh listener registration without waiting for the next activity event.

Phase 3.7f + 3.7h — Grace window

• iceBackoff.markFailure: while inside networkChangeGracePeriod (60s) of the most recent Reset(), cap the suspend delay at networkChangeRetryDelay (2s) and do NOT advance the long-term exponential schedule. Outside the window the normal 1-minute initial backoff applies.
• Window widened from initial 30s to 60s and delay reduced from 5s to 2s after observing real-world LTE-bounce behaviour: cold NAT mappings often need 3-4 ICE attempts to prime, and the previous 30s window only fit ~2 attempts.

Phase 3.7g — Skip ICE-Close on NC when still connected

• If ICE is still in Connected state when the network-change fires, don't call workerICE.Close() — the existing pair is fine, just refresh the listener.

#5805 cleanup — Guard-Loop fix

• worker_ice: skip new offers while the ICE agent is in Connecting. Drops the candidate-buffer approach the closed PR proposed in favour of a much simpler offer-skip during connecting.

GUI surface (Phase 3.7h GUI)

• client/cmd/service: persist connection-mode + timeouts on install/reconfigure.
• client/ui: Connection Mode + timeouts in Network tab.
• client: surface server-pushed connection-mode/timeouts via daemon-RPC.
• client/ui: "Follow-Server (currently: …)" display + remove the now-redundant Lazy menu.
• client/android: gomobile getters for ConnectionMode + ServerPushed values.

Tests

• go test ./client/internal/peer ./client/internal/peer/guard — pass (full ICE-backoff suite including the new grace-window cases).
• go build ./client/... — pass on linux/amd64, linux/arm64, windows/amd64.
• Hardware-validated against three OpenWrt routers (LAN host candidate, internet srflx, LTE srflx) plus a Windows 11 daemon. After a forced LTE-modem reset, the LTE peer recovers to P2P srflx within ~50 s instead of the 2-3 minutes the pre-3.7f code took.

Test plan

• Grace window: failure inside 60s → 2s retry; failure after 60s → exponential restarts at ~1m.
• No regression for the non-network-change failure path (still uses the exponential schedule).
• Signal/relay reconnect drives Reset() → grace window engages.
• Maintainer to verify: GUI changes on macOS / Linux ARM (I tested Windows + Linux/amd64 only).

Use case

A laptop that switches between docked Ethernet, office WiFi, and tethered LTE during the day. Without this PR every transition burns the exponential backoff and the user sees "stuck on relay" for 5-15 minutes. With this PR the post-handover recovery is bounded at ~50s.

Linked work

• Stacks on PR-A (Phase 1+2+3 foundation).
• Picks up the offer-skip-during-connecting fix from closed [client] Fix ICE reconnection loop and buffer candidates before agent initialization #5805 (the original PR's candidate-buffer approach is replaced with the simpler pattern).
• Closes part of Consolidate connection-mode flags; add p2p-dynamic and p2p-dynamic-lazy modes #5989.

Maintainers are welcome to push directly to this branch.

Summary by CodeRabbit

• New Features

  • Configurable connection modes (relay-forced, p2p, p2p-lazy, p2p-dynamic) with per-mode timeouts and local overrides.
  • Profile/install-time and CLI persistence for connection settings; server can push defaults surfaced to clients and UI.
  • ICE failure backoff with visible status (suspended, failures, next-retry) and graceful recovery.

• UI

  • Network/settings UI updated to manage connection mode, timeout fields, and show server-provided hints.

• Tests

  • Expanded test coverage across mode resolution, inactivity timers, ICE backoff, and related flows.

Documentation

• Documentation is not needed

These changes are internal lifecycle / behavioural improvements; no user-visible API or CLI flag added that warrants new public docs. Existing flags/Settings already documented at netbirdio/docs cover the surface area.

[coderabbitai]

Actionable comments posted: 18

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

client/internal/peer/guard/guard.go (1)

138-147: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Synchronous callback may stall the reconnect loop

onNetworkChange() is called directly inside the select body. If the callback takes any non-trivial time — or tries to acquire a lock that the surrounding Conn code also holds during network-change processing — ctx.Done(), relayedConnDisconnected, and iCEConnDisconnected events are all frozen until it returns.

Per the PR description the callback is expected to "reset iceBackoff + recreate workerICE", which touches Conn's internal mutex and may spawn goroutines. Consider dispatching asynchronously to keep the loop responsive:

💡 Suggested fix

-		case <-srReconnectedChan:
-			g.log.Debugf("has network changes, reset reconnection ticker")
-			ticker.Stop()
-			ticker = g.newReconnectTicker(ctx)
-			tickerChannel = ticker.C
-			iceState.reset()
-			// Phase 3.5 (`#5989`): notify Conn to reset iceBackoff + recreate workerICE
-			if g.onNetworkChange != nil {
-				g.onNetworkChange()
-			}
+		case <-srReconnectedChan:
+			g.log.Debugf("has network changes, reset reconnection ticker")
+			ticker.Stop()
+			ticker = g.newReconnectTicker(ctx)
+			tickerChannel = ticker.C
+			iceState.reset()
+			// Phase 3.5 (`#5989`): notify Conn to reset iceBackoff + recreate workerICE.
+			// Fire in a goroutine so the reconnect loop stays responsive to ctx.Done()
+			// and other disconnect events while the callback runs.
+			if g.onNetworkChange != nil {
+				go g.onNetworkChange()
+			}

🤖 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 `@client/internal/peer/guard/guard.go` around lines 138 - 147, The select case
calling g.onNetworkChange() synchronously can block the reconnect loop; change
it to invoke the callback asynchronously (e.g., spawn a goroutine) so the select
can continue handling ctx.Done(), relayedConnDisconnected and
iCEConnDisconnected immediately. Locate the case handling srReconnectedChan in
guard.go (references: srReconnectedChan, ticker, g.newReconnectTicker,
iceState.reset, g.onNetworkChange) and replace the direct call to
g.onNetworkChange() with a non-blocking dispatch (start a goroutine, optionally
recover/panic-handle and/or swap to a bounded worker or channel if you must
limit concurrency) to avoid holding up the loop.

🧹 Nitpick comments (5)

management/server/types/settings_test.go (1)

5-20: ⚡ Quick win

Consider adding a ConnectionMode deep-copy test to cover cloneStringPtr

The test validates cloneUint32Ptr via P2pRetryMaxSeconds, but cloneStringPtr (used by ConnectionMode) is a separate code path with no test coverage. A mirror test would be low-effort:

🧪 Suggested addition

+func TestSettings_Copy_ConnectionMode(t *testing.T) {
+	mode := "p2p-dynamic"
+	src := &Settings{ConnectionMode: &mode}
+	dst := src.Copy()
+	if dst.ConnectionMode == nil {
+		t.Fatal("Copy lost ConnectionMode pointer")
+	}
+	if *dst.ConnectionMode != "p2p-dynamic" {
+		t.Fatalf("expected p2p-dynamic, got %s", *dst.ConnectionMode)
+	}
+	// Verify deep copy (different pointers).
+	*dst.ConnectionMode = "relay-forced"
+	if *src.ConnectionMode != "p2p-dynamic" {
+		t.Fatal("Copy did not deep-clone ConnectionMode")
+	}
+}

🤖 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 `@management/server/types/settings_test.go` around lines 5 - 20, Add a new unit
test mirroring TestSettings_Copy_P2pRetryMaxSeconds to validate deep-copy
behavior for Settings.ConnectionMode: create a source Settings with
ConnectionMode set to a string pointer, call Settings.Copy(), assert
dst.ConnectionMode is non-nil and equals the original value, then mutate
*dst.ConnectionMode and assert *src.ConnectionMode remains unchanged to verify
cloneStringPtr produced a deep copy; reference the Settings struct, its Copy
method, and cloneStringPtr in the test.

shared/management/http/api/openapi.yml (1)

357-369: ⚡ Quick win

Mark legacy lazy_connection_enabled as deprecated and state precedence.

With both fields present, migration intent is unclear unless deprecation and precedence are explicit in the schema.

🧭 Suggested compatibility clarification

         lazy_connection_enabled:
           x-experimental: true
+          deprecated: true
           description: Enables or disables experimental lazy connection
           type: boolean
           example: true
         connection_mode:
           x-experimental: true
           type: string
           enum: [relay-forced, p2p, p2p-lazy, p2p-dynamic]
           nullable: true
           description: |
             Account-wide default peer-connection mode. NULL means
             "fall back to lazy_connection_enabled" for backwards compatibility.
+            When non-NULL, `connection_mode` takes precedence.

🤖 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 `@shared/management/http/api/openapi.yml` around lines 357 - 369, Mark the
legacy boolean field lazy_connection_enabled as deprecated and make precedence
explicit: add deprecated: true to the lazy_connection_enabled schema and update
its description to indicate it is legacy and overridden by connection_mode when
connection_mode is non-null; update the connection_mode description to
explicitly state that a non-null connection_mode takes precedence over
lazy_connection_enabled and that NULL means "fall back to
lazy_connection_enabled" for backwards compatibility (use the existing keys
lazy_connection_enabled and connection_mode to locate the schema).

shared/connectionmode/mode_test.go (1)

77-94: 💤 Low value

ModeFollowServer missing from TestToLazyConnectionEnabled.

The table omits ModeFollowServer, so its ToLazyConnectionEnabled() return value is never asserted. If its behaviour changes it won't be caught. Consider adding a case (expected value is almost certainly false, matching ModeUnspecified).

🤖 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 `@shared/connectionmode/mode_test.go` around lines 77 - 94, The test
TestToLazyConnectionEnabled omits ModeFollowServer so its
ToLazyConnectionEnabled() behavior isn't asserted; add a test case entry in the
cases table for ModeFollowServer with the expected boolean (likely false,
matching ModeUnspecified) so the call to Mode.ToLazyConnectionEnabled() for
ModeFollowServer is validated like the other entries.

client/internal/peer/status.go (1)

367-382: 💤 Low value

UpdatePeerIceBackoff silently ignores unknown peer keys — inconsistent with other update methods.

Every other UpdatePeer* method (e.g., UpdatePeerState, UpdateWireGuardPeerState, UpdatePeerFQDN) returns an error when the peer is not found. UpdatePeerIceBackoff returns nothing and silently no-ops. While a silent no-op is safe here (the peer may have been removed between the ICE failure and this call), the divergence from the established pattern makes it harder to detect misconfigured call sites during development. Consider returning an error for consistency, or add a brief comment explaining why the silent no-op is intentional.

🤖 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 `@client/internal/peer/status.go` around lines 367 - 382, UpdatePeerIceBackoff
currently silently no-ops when the peer key is missing, diverging from other
update methods like UpdatePeerState / UpdateWireGuardPeerState / UpdatePeerFQDN
which return an error; change UpdatePeerIceBackoff(pubKey string, snap
BackoffSnapshot) to return an error, lock/unlock as before, and when peer not
found return a descriptive error (e.g., fmt.Errorf("peer %s not found",
pubKey)); update callers to handle the returned error, or if you intentionally
want the silent no-op instead, add a short comment above UpdatePeerIceBackoff
explaining why ignoring missing peers is acceptable to avoid surprise.

client/internal/peer/conn_test.go (1)

374-384: 💤 Low value

Test assertion is tied to an internal error string.

Line 381 checks strings.Contains(errMsg, "workerICE") — if the error message in AttachICE is ever rewarded, this assertion passes vacuously. Consider asserting on a sentinel error value (errors.Is) instead of a substring, or at minimum add a comment acknowledging the coupling.

🤖 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 `@client/internal/peer/conn_test.go` around lines 374 - 384, The test currently
asserts on the error string from c.AttachICE() by checking for "workerICE",
which is brittle; change the implementation of AttachICE to return or wrap a
sentinel error (e.g., ErrNilWorkerICE) and update the test to use errors.Is(err,
ErrNilWorkerICE) instead of substring matching; if adding a sentinel isn't
possible right now, at minimum replace the substring check with errors.Is
against an existing exported sentinel or add a clear comment documenting the
coupling. Ensure AttachICE (and any callers) wrap the sentinel with
fmt.Errorf("%w: ...", ErrNilWorkerICE) so errors.Is will work reliably.

🤖 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 `@client/android/preferences.go`:
- Around line 347-352: Validate negative timeout inputs before casting to
uint32: in SetRelayTimeoutSeconds (and the other two timeout setters around the
same area) check if the incoming secs < 0 and, if so, clear the override (set
the corresponding p.configInput.*TimeoutSeconds = nil) instead of casting;
otherwise cast to uint32 and assign as before. Ensure you update all three
setters (the SetRelayTimeoutSeconds function and the two other timeout setter
functions referenced) with the same guard.

In `@client/cmd/up.go`:
- Around line 442-453: The SetConfig handler in server.go is ignoring the
incoming optional fields (req.ConnectionMode, req.RelayTimeoutSeconds,
req.P2PTimeoutSeconds, req.P2PRetryMaxSeconds) so daemon-mode client overrides
are dropped; add four nil-checks in server.go's SetConfig to apply each field
when non-nil (e.g., if req.ConnectionMode != nil { serverConfig.ConnectionMode =
*req.ConnectionMode }) and persist/update the server configuration/store
accordingly; do the same for Login's handler to read and persist the same four
fields from the incoming request (setupSetConfigReq / setupLoginRequest populate
them) so the client flags are honored.

In `@client/internal/conn_mgr.go`:
- Around line 277-320: The code updates ConnMgr.relayTimeoutSecs, p2pTimeoutSecs
and p2pRetryMaxSecs before deciding whether to restart the existing lazy
manager, so changes to timeouts inside the same managed mode are ignored; fix by
capturing whether the timeout values actually changed (e.g. timeoutsChanged :=
newRelay != e.relayTimeoutSecs || newP2P != e.p2pTimeoutSecs || newP2pRetry !=
e.p2pRetryMaxSecs) before overwriting the fields, then after setting e.mode and
the timeout fields, if isManaged && e.lazyConnMgr != nil && timeoutsChanged call
e.closeManager(ctx), e.statusRecorder.UpdateLazyConnection(false), and then
re-init via e.initLazyManager(ctx) followed by e.startModeSideEffects() and
e.addPeersToLazyConnManager(); keep existing logic for mode flips using
modeUsesLazyMgr, and ensure propagateP2pRetryMaxToConns() is still called after
updating the retry field.
- Around line 176-188: When Rosenpass is enabled we must not leave e.mode set to
a lazy/dynamic variant while lazyConnMgr is not started; change ConnMgr.Start so
that if e.rosenpassEnabled is true you replace/downgrade e.mode to a non-lazy
equivalent (e.g., convert p2p-lazy/p2p-dynamic -> their non-lazy counterpart)
before returning, and also update UpdatedRemotePeerConfig to detect when newMode
is a lazy/dynamic variant while e.rosenpassEnabled is true and store the
downgraded non-lazy mode instead of newMode; reference ConnMgr.Start,
e.rosenpassEnabled, e.mode, lazyConnMgr, UpdatedRemotePeerConfig, newMode and
ActivatePeer when making this change.

In `@client/internal/lazyconn/inactivity/manager.go`:
- Around line 233-239: The scan can enqueue the same peer into both iceIdle and
relayIdle, causing races in the consumer; modify the manager scan so that when a
peer meets relayTimeout it is added to relayIdle and then skipped for ICE
notification (i.e., check relayIdle before adding to iceIdle), or alternatively
collapse to a single ordered notification list processed in priority order;
specifically update the block that uses m.relayTimeout and m.iceTimeout
(variables shown as relayIdle, iceIdle, checkTime, since) so relayIdle is
evaluated/assigned first and iceIdle insertion is gated on relayIdle not
containing peerID (or replace both maps with a single ordered notification for
the consumer).
- Around line 76-87: NewManagerWithTwoTimers currently accepts negative or
sub-minute durations; validate and clamp iceTimeout and relayTimeout the same as
the legacy constructor by enforcing MinimumInactivityThreshold (and preventing
negatives) before calling newManager. In NewManagerWithTwoTimers, if iceTimeout
or relayTimeout is less than MinimumInactivityThreshold (or negative), set them
to MinimumInactivityThreshold, log the adjusted values for visibility (e.g.,
"adjusted ICE/relay inactivity timeout"), and then pass the sanitized timeouts
into newManager so both constructors behave identically.

In `@client/internal/lazyconn/manager/manager.go`:
- Around line 103-109: The code treats iceTO==0 && relayTO==0 as a legacy "not
migrated" case; change resolvedTimeouts() to also return a migration flag (e.g.,
resolvedTimeouts() -> (iceTO, relayTO, migrated bool)) and then update the
branch in manager.go to: if !migrated { keep legacy single-timer behavior via
inactivity.NewManager(wgIface, config.InactivityThreshold) } else if iceTO == 0
&& relayTO == 0 { explicitly disable inactivity (e.g., set m.inactivityManager
to a disabled/no-op manager or nil) } else { use
inactivity.NewManagerWithTwoTimers(wgIface, iceTO, relayTO) } so intentional 0/0
does not fall back to the legacy manager; refer to resolvedTimeouts, iceTO,
relayTO, m.inactivityManager, NewManager and NewManagerWithTwoTimers when making
the change.

In `@client/internal/peer/conn.go`:
- Around line 202-212: The code currently conflates three states (disabled
sentinel, explicit 0 for "use default", and a literal non-zero seconds) so
Open() loses a prior "disabled backoff" sentinel; update the Open()/backoff init
logic and SetIceBackoffMax handling to preserve the explicit disable sentinel:
when reading conn.config.P2pRetryMaxSeconds in the backoff initialization
(backoffCap calculation and calls to newIceBackoff/SetMaxBackoff) first test for
the special disable sentinel value (e.g. ^uint32(0) or whatever sentinel you
use) and propagate that as a disabled state to conn.iceBackoff (e.g. leave
conn.iceBackoff as disabled or call a Disable/SetMaxBackoffDisabled path)
instead of treating it as a huge duration; likewise, make SetIceBackoffMax
distinguish between setting the disable sentinel, explicit 0 (meaning use
default cap), and non-zero seconds so it does not rewrite a disable sentinel
into 0 and thereby lose the disabled state across Close/Open cycles.

In `@client/internal/peer/env_test.go`:
- Line 25: Update the test case name string literal "connection_mode unparseable
falls through to legacy" to use the preferred spelling "unparsable" (i.e. change
"unparseable" → "unparsable") in the table entry currently containing
{"connection_mode unparseable falls through to legacy", "garbage", "true", "",
"", connectionmode.ModeRelayForced, 0} so that the test identifier matches
codespell expectations.

In `@client/internal/peer/status.go`:
- Around line 1372-1374: The IceBackoffNextRetry field is being set with
timestamppb.New even when peerState.IceBackoffNextRetry is the zero time, which
produces an invalid past proto timestamp; change the assignment that sets
IceBackoffNextRetry (the struct population using peerState) to guard with
peerState.IceBackoffNextRetry.IsZero() and only call timestamppb.New(...) when
non-zero, otherwise set the proto field to nil so consumers see an unset
timestamp; locate the code that sets IceBackoffNextRetry alongside
IceBackoffFailures/IceBackoffSuspended in status.go and apply this conditional.

In `@client/internal/peer/worker_ice.go`:
- Around line 218-231: The read/write of lastKnownState races because
IsConnected() reads it under muxAgent while onConnectionStateChange writes it
without locking; change lastKnownState to an atomic.Int32 (or int32 accessed via
sync/atomic) and replace writes in onConnectionStateChange with
atomic.StoreInt32(&w.lastKnownState, int32(ice.ConnectionStateX)) and the read
in IsConnected() with atomic.LoadInt32(&w.lastKnownState) (while still using
muxAgent to check w.agent != nil), or alternatively guard the writes in
onConnectionStateChange with muxAgent; update all places that set/read
lastKnownState to use the chosen atomic or lock-based approach to eliminate the
data race.

In `@client/server/server.go`:
- Around line 1518-1538: The comment claiming "all zero/empty when the engine
has not received PeerConfig" is incorrect because
ConnMgr.ServerPushedP2pRetryMaxSecs() substitutes a built-in default, so
spP2pRetMax will be non‑zero; fix this by either updating the comment to reflect
the actual semantics or (preferred) changing the getter behavior: modify
ConnMgr.ServerPushedP2pRetryMaxSecs() (in conn_mgr.go) to return 0 when the
stored peer-config value is unset instead of substituting the default, and keep
default application logic elsewhere (UI or a helper), so the code in server.go
that reads spP2pRetMax from cm.ServerPushedP2pRetryMaxSecs() can reliably use 0
as the "not yet received" sentinel.

In `@client/ui/client_ui.go`:
- Around line 656-666: parseUint32Field currently coerces empty, non-numeric,
and overflowed input to 0 which lets buildSetConfigRequest silently clear
overrides; change parseUint32Field to return (uint32, error) (or (uint32, bool)
if you prefer) and treat empty input as a distinct "no value" case rather than
returning 0, return a parse/overflow error for invalid inputs, and then update
buildSetConfigRequest (and the other caller around lines 741-750) to
propagate/handle the error: reject the request or show validation feedback
instead of sending 0 to the daemon. Ensure you validate numeric range (32-bit)
and non-digit characters in parseUint32Field and adjust all call sites to handle
the new return signature.

In `@management/internals/shared/grpc/conversion.go`:
- Around line 115-119: When parsing Settings.ConnectionMode inside the if
settings.ConnectionMode != nil block use
connectionmode.ParseString(*settings.ConnectionMode) as before but if it returns
an error emit a one-line warning indicating the bad value so admin typos are
visible; update the code around connectionmode.ParseString to log a warn (e.g.,
logger.Warnf or the package logger) with the invalid string and the parse error
when err != nil, otherwise keep the existing behavior of assigning resolvedMode
when m != connectionmode.ModeUnspecified.
- Around line 26-29: The initial peer connection path doesn't translate the
wire-format sentinel (p2pRetryMaxDisabledSentinel) to zero, so Conn.Open()
computes backoffCap from conn.config.P2pRetryMaxSeconds and treats the sentinel
as a huge duration; update Conn.Open() to detect if
conn.config.P2pRetryMaxSeconds equals p2pRetryMaxDisabledSentinel and treat it
as 0 (or alternatively perform the same sentinel→0 conversion earlier in
resolveConnectionMode() / engine.createPeerConnConfig() /
ConnMgr.P2pRetryMax()), then compute backoffCap :=
time.Duration(uint32ValueConvertedToZeroIfSentinel) * time.Second so sentinel
yields duration 0.

In `@management/server/account.go`:
- Around line 372-375: The change handlers currently only audit connection-mode
related updates but do not push new runtime config to live peers; update the
caller to trigger am.updateAccountPeers when any of ConnectionMode,
RelayTimeoutSeconds, P2pTimeoutSeconds, or P2pRetryMaxSeconds change.
Concretely: modify handleConnectionModeSettings (or have the handlers return a
bool) so the outer code that calls am.handleConnectionModeSettings(...) can
detect a change and then call am.updateAccountPeers(ctx, userID, accountID) (or
the existing updateAccountPeers signature) after the settings are saved/audited;
ensure the check compares oldSettings.ConnectionMode,
oldSettings.RelayTimeoutSeconds, oldSettings.P2pTimeoutSeconds, and
oldSettings.P2pRetryMaxSeconds against the newSettings versions and only calls
updateAccountPeers when any differ.

In `@management/server/http/handlers/accounts/accounts_handler.go`:
- Around line 229-240: The three timeout fields (req.Settings.P2pTimeoutSeconds,
P2pRetryMaxSeconds, RelayTimeoutSeconds) are being cast directly to uint32 which
allows negative JSON numbers to wrap to large unsigned values; update the
validation before casting in the handler that populates returnSettings: check
each pointer is non-nil and *value >= 0, and if a negative value is detected
return a validation/error response (or handle it per existing validation flow)
instead of casting; then safely convert to uint32 (e.g., v :=
uint32(*req.Settings.P2pTimeoutSeconds)) and assign to
returnSettings.P2pTimeoutSeconds, P2pRetryMaxSeconds, and RelayTimeoutSeconds
respectively.

In `@shared/management/http/api/openapi.yml`:
- Around line 367-372: The description for the "connection_mode" enum is
outdated and still states that "p2p-dynamic" is reserved/passes through; update
the OpenAPI description for the connection_mode field so it reflects the current
phase behavior (remove the claim that p2p-dynamic is reserved/pass-through) and
clearly document which modes are currently functional (relay-forced, p2p,
p2p-lazy) and how NULL interacts with lazy_connection_enabled for backwards
compatibility; edit the description text near the connection_mode property in
shared/management/http/api/openapi.yml and mention the accurate phase semantics
for p2p-dynamic (omit or mark as not supported rather than pass-through) and
ensure references to lazy_connection_enabled and Phase 1/Phase 2 are consistent.

---

Outside diff comments:
In `@client/internal/peer/guard/guard.go`:
- Around line 138-147: The select case calling g.onNetworkChange() synchronously
can block the reconnect loop; change it to invoke the callback asynchronously
(e.g., spawn a goroutine) so the select can continue handling ctx.Done(),
relayedConnDisconnected and iCEConnDisconnected immediately. Locate the case
handling srReconnectedChan in guard.go (references: srReconnectedChan, ticker,
g.newReconnectTicker, iceState.reset, g.onNetworkChange) and replace the direct
call to g.onNetworkChange() with a non-blocking dispatch (start a goroutine,
optionally recover/panic-handle and/or swap to a bounded worker or channel if
you must limit concurrency) to avoid holding up the loop.

---

Nitpick comments:
In `@client/internal/peer/conn_test.go`:
- Around line 374-384: The test currently asserts on the error string from
c.AttachICE() by checking for "workerICE", which is brittle; change the
implementation of AttachICE to return or wrap a sentinel error (e.g.,
ErrNilWorkerICE) and update the test to use errors.Is(err, ErrNilWorkerICE)
instead of substring matching; if adding a sentinel isn't possible right now, at
minimum replace the substring check with errors.Is against an existing exported
sentinel or add a clear comment documenting the coupling. Ensure AttachICE (and
any callers) wrap the sentinel with fmt.Errorf("%w: ...", ErrNilWorkerICE) so
errors.Is will work reliably.

In `@client/internal/peer/status.go`:
- Around line 367-382: UpdatePeerIceBackoff currently silently no-ops when the
peer key is missing, diverging from other update methods like UpdatePeerState /
UpdateWireGuardPeerState / UpdatePeerFQDN which return an error; change
UpdatePeerIceBackoff(pubKey string, snap BackoffSnapshot) to return an error,
lock/unlock as before, and when peer not found return a descriptive error (e.g.,
fmt.Errorf("peer %s not found", pubKey)); update callers to handle the returned
error, or if you intentionally want the silent no-op instead, add a short
comment above UpdatePeerIceBackoff explaining why ignoring missing peers is
acceptable to avoid surprise.

In `@management/server/types/settings_test.go`:
- Around line 5-20: Add a new unit test mirroring
TestSettings_Copy_P2pRetryMaxSeconds to validate deep-copy behavior for
Settings.ConnectionMode: create a source Settings with ConnectionMode set to a
string pointer, call Settings.Copy(), assert dst.ConnectionMode is non-nil and
equals the original value, then mutate *dst.ConnectionMode and assert
*src.ConnectionMode remains unchanged to verify cloneStringPtr produced a deep
copy; reference the Settings struct, its Copy method, and cloneStringPtr in the
test.

In `@shared/connectionmode/mode_test.go`:
- Around line 77-94: The test TestToLazyConnectionEnabled omits ModeFollowServer
so its ToLazyConnectionEnabled() behavior isn't asserted; add a test case entry
in the cases table for ModeFollowServer with the expected boolean (likely false,
matching ModeUnspecified) so the call to Mode.ToLazyConnectionEnabled() for
ModeFollowServer is validated like the other entries.

In `@shared/management/http/api/openapi.yml`:
- Around line 357-369: Mark the legacy boolean field lazy_connection_enabled as
deprecated and make precedence explicit: add deprecated: true to the
lazy_connection_enabled schema and update its description to indicate it is
legacy and overridden by connection_mode when connection_mode is non-null;
update the connection_mode description to explicitly state that a non-null
connection_mode takes precedence over lazy_connection_enabled and that NULL
means "fall back to lazy_connection_enabled" for backwards compatibility (use
the existing keys lazy_connection_enabled and connection_mode to locate the
schema).

🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f6253fba-a00e-49ab-b17b-28539d6a1fa2

📥 Commits

Reviewing files that changed from the base of the PR and between b19b746 and 2b6f78b.

⛔ Files ignored due to path filters (4)

• client/proto/daemon.pb.go is excluded by !**/*.pb.go
• client/proto/daemon_grpc.pb.go is excluded by !**/*.pb.go
• shared/management/proto/management.pb.go is excluded by !**/*.pb.go
• shared/management/proto/proxy_service.pb.go is excluded by !**/*.pb.go

📒 Files selected for processing (46)

• client/android/client.go
• client/android/preferences.go
• client/cmd/root.go
• client/cmd/service.go
• client/cmd/service_installer.go
• client/cmd/up.go
• client/internal/conn_mgr.go
• client/internal/conn_mgr_test.go
• client/internal/connect.go
• client/internal/engine.go
• client/internal/lazyconn/env.go
• client/internal/lazyconn/inactivity/manager.go
• client/internal/lazyconn/inactivity/manager_test.go
• client/internal/lazyconn/manager/manager.go
• client/internal/peer/conn.go
• client/internal/peer/conn_test.go
• client/internal/peer/env.go
• client/internal/peer/env_test.go
• client/internal/peer/guard/guard.go
• client/internal/peer/handshaker.go
• client/internal/peer/handshaker_test.go
• client/internal/peer/ice_backoff.go
• client/internal/peer/ice_backoff_test.go
• client/internal/peer/status.go
• client/internal/peer/worker_ice.go
• client/internal/profilemanager/config.go
• client/proto/daemon.proto
• client/server/server.go
• client/status/status.go
• client/status/status_test.go
• client/ui/client_ui.go
• client/ui/const.go
• client/ui/event_handler.go
• management/internals/shared/grpc/conversion.go
• management/internals/shared/grpc/conversion_test.go
• management/server/account.go
• management/server/activity/codes.go
• management/server/http/handlers/accounts/accounts_handler.go
• management/server/http/handlers/accounts/accounts_handler_test.go
• management/server/types/settings.go
• management/server/types/settings_test.go
• shared/connectionmode/mode.go
• shared/connectionmode/mode_test.go
• shared/management/http/api/openapi.yml
• shared/management/http/api/types.gen.go
• shared/management/proto/management.proto

💤 Files with no reviewable changes (2)

• client/ui/event_handler.go
• client/ui/const.go

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard negative timeout inputs before casting to uint32.

These setters currently turn negative Android values into huge positive timeouts (-1 becomes 4294967295), which silently corrupts the persisted override instead of clearing/rejecting it. Clamp or reject < 0 before the cast for all three setters.

🛠️ Suggested fix

 func (p *Preferences) SetRelayTimeoutSeconds(secs int64) {
+	if secs < 0 {
+		secs = 0
+	}
 	v := uint32(secs)
 	p.configInput.RelayTimeoutSeconds = &v
 }
@@
 func (p *Preferences) SetP2pTimeoutSeconds(secs int64) {
+	if secs < 0 {
+		secs = 0
+	}
 	v := uint32(secs)
 	p.configInput.P2pTimeoutSeconds = &v
 }
@@
 func (p *Preferences) SetP2pRetryMaxSeconds(secs int64) {
+	if secs < 0 {
+		secs = 0
+	}
 	v := uint32(secs)
 	p.configInput.P2pRetryMaxSeconds = &v
 }

Also applies to: 368-373, 388-393

🤖 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 `@client/android/preferences.go` around lines 347 - 352, Validate negative
timeout inputs before casting to uint32: in SetRelayTimeoutSeconds (and the
other two timeout setters around the same area) check if the incoming secs < 0
and, if so, clear the override (set the corresponding
p.configInput.*TimeoutSeconds = nil) instead of casting; otherwise cast to
uint32 and assign as before. Ensure you update all three setters (the
SetRelayTimeoutSeconds function and the two other timeout setter functions
referenced) with the same guard.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

New connection-mode flags are silently dropped in daemon mode.

setupSetConfigReq populates req.ConnectionMode, req.RelayTimeoutSeconds, req.P2PTimeoutSeconds, and req.P2PRetryMaxSeconds, but server.go's SetConfig handler never reads or persists these four fields from the incoming *proto.SetConfigRequest. In daemon mode the client.SetConfig call at runInDaemonMode succeeds with no error, but the values are silently discarded — the user's --connection-mode override never takes effect.

The same applies to setupLoginRequest (lines 699-710): the populated fields are carried over the wire but server.go's Login handler does not persist them either.

The fix is to extract and apply these fields inside server.go's SetConfig:

🐛 Proposed fix in server.go `SetConfig`

 	if msg.Mtu != nil {
 		mtu := uint16(*msg.Mtu)
 		config.MTU = &mtu
 	}
+
+	if msg.ConnectionMode != nil {
+		config.ConnectionMode = *msg.ConnectionMode
+	}
+	if msg.RelayTimeoutSeconds != nil {
+		config.RelayTimeoutSeconds = *msg.RelayTimeoutSeconds
+	}
+	if msg.P2PTimeoutSeconds != nil {
+		config.P2pTimeoutSeconds = *msg.P2PTimeoutSeconds
+	}
+	if msg.P2PRetryMaxSeconds != nil {
+		config.P2pRetryMaxSeconds = *msg.P2PRetryMaxSeconds
+	}

 	if _, err := profilemanager.UpdateConfig(config); err != nil {

Wait - this is actually not high effort. The fix is 4 simple nil-checks in one location.

🤖 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 `@client/cmd/up.go` around lines 442 - 453, The SetConfig handler in server.go
is ignoring the incoming optional fields (req.ConnectionMode,
req.RelayTimeoutSeconds, req.P2PTimeoutSeconds, req.P2PRetryMaxSeconds) so
daemon-mode client overrides are dropped; add four nil-checks in server.go's
SetConfig to apply each field when non-nil (e.g., if req.ConnectionMode != nil {
serverConfig.ConnectionMode = *req.ConnectionMode }) and persist/update the
server configuration/store accordingly; do the same for Login's handler to read
and persist the same four fields from the incoming request (setupSetConfigReq /
setupLoginRequest populate them) so the client flags are honored.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Restart the lazy manager when timeouts change inside the same managed mode.

relayTimeoutSecs and p2pTimeoutSecs are only consumed in initLazyManager(). Right now a server push from, say, p2p-dynamic(60/30) to p2p-dynamic(10/5) updates the fields on ConnMgr but keeps the existing manager instance alive, so the inactivity thresholds never actually change until a full restart or a mode flip.

Suggested fix

-	if modeChanged && wasManaged && isManaged {
+	timeoutsChanged := newRelay != e.relayTimeoutSecs || newP2P != e.p2pTimeoutSecs
+	if wasManaged && isManaged && (modeChanged || timeoutsChanged) {
 		// Switching between lazy and dynamic at runtime: tear down the
 		// existing manager so initLazyManager picks up the new timeouts.
-		log.Infof("lazy/dynamic mode change %s -> %s, restarting manager", prev, newMode)
+		log.Infof("restarting lazy/dynamic manager to apply updated mode/timeouts (%s -> %s)", prev, newMode)
 		e.closeManager(ctx)
 		e.statusRecorder.UpdateLazyConnection(false)
 	}

Capture timeoutsChanged before overwriting the old timeout fields so the comparison is against the previous manager config.

Also applies to: 553-563

🤖 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 `@client/internal/conn_mgr.go` around lines 277 - 320, The code updates
ConnMgr.relayTimeoutSecs, p2pTimeoutSecs and p2pRetryMaxSecs before deciding
whether to restart the existing lazy manager, so changes to timeouts inside the
same managed mode are ignored; fix by capturing whether the timeout values
actually changed (e.g. timeoutsChanged := newRelay != e.relayTimeoutSecs ||
newP2P != e.p2pTimeoutSecs || newP2pRetry != e.p2pRetryMaxSecs) before
overwriting the fields, then after setting e.mode and the timeout fields, if
isManaged && e.lazyConnMgr != nil && timeoutsChanged call e.closeManager(ctx),
e.statusRecorder.UpdateLazyConnection(false), and then re-init via
e.initLazyManager(ctx) followed by e.startModeSideEffects() and
e.addPeersToLazyConnManager(); keep existing logic for mode flips using
modeUsesLazyMgr, and ensure propagateP2pRetryMaxToConns() is still called after
updating the retry field.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate two-timer thresholds the same way the legacy constructor does.

NewManagerWithTwoTimers bypasses MinimumInactivityThreshold, so negative or sub-minute values are accepted on the new path. With a 1-minute polling loop, that makes small values misleading and negative values effectively mark every peer idle on the first tick.

🛠️ Suggested fix

 func NewManagerWithTwoTimers(iface WgInterface, iceTimeout, relayTimeout time.Duration) *Manager {
+	validateOptional := func(name string, v time.Duration) time.Duration {
+		if v == 0 {
+			return 0
+		}
+		if v < MinimumInactivityThreshold {
+			log.Warnf("%s inactivity timeout %v is too low, using minimum %v", name, v, MinimumInactivityThreshold)
+			return MinimumInactivityThreshold
+		}
+		return v
+	}
+
+	iceTimeout = validateOptional("ICE", iceTimeout)
+	relayTimeout = validateOptional("relay", relayTimeout)
+
 	if iceTimeout > 0 {
 		log.Infof("ICE inactivity timeout: %v", iceTimeout)
 	}
 	if relayTimeout > 0 {

🤖 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 `@client/internal/lazyconn/inactivity/manager.go` around lines 76 - 87,
NewManagerWithTwoTimers currently accepts negative or sub-minute durations;
validate and clamp iceTimeout and relayTimeout the same as the legacy
constructor by enforcing MinimumInactivityThreshold (and preventing negatives)
before calling newManager. In NewManagerWithTwoTimers, if iceTimeout or
relayTimeout is less than MinimumInactivityThreshold (or negative), set them to
MinimumInactivityThreshold, log the adjusted values for visibility (e.g.,
"adjusted ICE/relay inactivity timeout"), and then pass the sanitized timeouts
into newManager so both constructors behave identically.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Locate where the wire P2PRetryMaxSeconds is consumed on the daemon side and
# check that both 0 and ^uint32(0) are handled.
rg -nP --type=go -C5 'P2PRetryMaxSeconds|P2pRetryMaxSeconds'
echo "---"
# Look for the sentinel constant or any uint32-max comparison in client code.
rg -nP --type=go -C3 '\^uint32\(0\)|math\.MaxUint32|0xffffffff'

Repository: netbirdio/netbird

Length of output: 50373

---

🏁 Script executed:

# Search for code that interprets the sentinel or handles uint32-max specially
rg -nP --type=go 'SetMaxBackoff|SetIceBackoffMax' -A 5 -B 2
echo "---"
# Search in engine or relevant client files for sentinel detection
rg -nP --type=go -C 3 'uint32.*max|math\.MaxUint32|\^uint32' client/internal/
echo "---"
# Look for where P2pRetryMaxSeconds is converted from wire format to time.Duration
rg -nP --type=go 'time\.Duration.*P2pRetryMax|P2pRetryMax.*time\.Duration' -A 2 -B 2

Repository: netbirdio/netbird

Length of output: 7903

---

🏁 Script executed:

sed -n '577,594p' client/internal/conn_mgr.go
echo "---"
# Also check engine.go for sentinel handling
rg -nP --type=go 'sentinelDisabled|uint32.*0\)|disable.*backoff' client/internal/engine.go -A 3 -B 3

Repository: netbirdio/netbird

Length of output: 543

---

🏁 Script executed:

# Find where P2pRetryMaxSeconds is initially set in engine config
rg -nP --type=go 'P2pRetryMaxSeconds:' client/internal/engine.go -B 5 -A 2
echo "---"
# Check resolveConnectionMode for sentinel handling  
sed -n '109,170p' client/internal/conn_mgr.go
echo "---"
# Look for any conversion before the value reaches Conn.config
rg -nP --type=go 'connConfig.*P2p|PeerConnConfig.*P2p' client/internal/engine.go -B 2 -A 2

Repository: netbirdio/netbird

Length of output: 2418

---

Verify where the wire-format sentinel (^uint32(0)) is converted to disable-backoff (duration 0) in the initial peer-connection path.

The server-side conversion (nil→0, &0→sentinel, &N→N) is correct, and propagateP2pRetryMaxToConns() properly handles sentinel→0 for subsequent server updates. However, the initial Conn.Open() path at peer/conn.go:204 lacks sentinel detection: backoffCap := time.Duration(conn.config.P2pRetryMaxSeconds) * time.Second will interpret sentinel as ~136 years instead of zero. The comment at peer/conn.go:98–99 claims "the resolver translates to time.Duration(0) at engine.go" but no such conversion occurs in resolveConnectionMode() or engine.createPeerConnConfig().

Either add sentinel-to-zero conversion before backoffCap is computed in Conn.Open(), or ensure the sentinel is translated in the initial config-resolution chain (e.g., in resolveConnectionMode() or ConnMgr.P2pRetryMax()).

🤖 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 `@management/internals/shared/grpc/conversion.go` around lines 26 - 29, The
initial peer connection path doesn't translate the wire-format sentinel
(p2pRetryMaxDisabledSentinel) to zero, so Conn.Open() computes backoffCap from
conn.config.P2pRetryMaxSeconds and treats the sentinel as a huge duration;
update Conn.Open() to detect if conn.config.P2pRetryMaxSeconds equals
p2pRetryMaxDisabledSentinel and treat it as 0 (or alternatively perform the same
sentinel→0 conversion earlier in resolveConnectionMode() /
engine.createPeerConnConfig() / ConnMgr.P2pRetryMax()), then compute backoffCap
:= time.Duration(uint32ValueConvertedToZeroIfSentinel) * time.Second so sentinel
yields duration 0.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Log when Settings.ConnectionMode fails to parse so admin typos are surfaced.

Right now an invalid value (say "p2p-typoed") is silently dropped — the function falls back to the legacy bool with no signal at all. A misconfigured admin will see "old" behavior with no clue why. A one-line warn log on the parse error is cheap and matches what the env-resolver in client/internal/peer/env.go already does for EnvKeyNBConnectionMode.

📝 Suggested change

 	resolvedMode := connectionmode.ResolveLegacyLazyBool(settings.LazyConnectionEnabled)
 	if settings.ConnectionMode != nil {
-		if m, err := connectionmode.ParseString(*settings.ConnectionMode); err == nil && m != connectionmode.ModeUnspecified {
+		m, err := connectionmode.ParseString(*settings.ConnectionMode)
+		if err != nil {
+			log.Warnf("ignoring unknown Settings.ConnectionMode %q: %v", *settings.ConnectionMode, err)
+		} else if m != connectionmode.ModeUnspecified {
 			resolvedMode = m
 		}
 	}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if settings.ConnectionMode != nil {
	if m, err := connectionmode.ParseString(*settings.ConnectionMode); err == nil && m != connectionmode.ModeUnspecified {
	resolvedMode = m
	}
	}
	if settings.ConnectionMode != nil {
	m, err := connectionmode.ParseString(*settings.ConnectionMode)
	if err != nil {
	log.Warnf("ignoring unknown Settings.ConnectionMode %q: %v", *settings.ConnectionMode, err)
	} else if m != connectionmode.ModeUnspecified {
	resolvedMode = m
	}
	}

🤖 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 `@management/internals/shared/grpc/conversion.go` around lines 115 - 119, When
parsing Settings.ConnectionMode inside the if settings.ConnectionMode != nil
block use connectionmode.ParseString(*settings.ConnectionMode) as before but if
it returns an error emit a one-line warning indicating the bad value so admin
typos are visible; update the code around connectionmode.ParseString to log a
warn (e.g., logger.Warnf or the package logger) with the invalid string and the
parse error when err != nil, otherwise keep the existing behavior of assigning
resolvedMode when m != connectionmode.ModeUnspecified.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Propagate connection-mode setting changes to peers, not just audit them.

This new audit hook lands, but updateAccountPeers is never flipped when ConnectionMode, RelayTimeoutSeconds, P2pTimeoutSeconds, or P2pRetryMaxSeconds change. As written, the account setting is saved and audited, but already-connected clients won't be pushed the new runtime config until some unrelated peer update happens.

🛠️ Suggested fix

 		if oldSettings.RoutingPeerDNSResolutionEnabled != newSettings.RoutingPeerDNSResolutionEnabled ||
 			oldSettings.LazyConnectionEnabled != newSettings.LazyConnectionEnabled ||
+			!equalStringPtr(oldSettings.ConnectionMode, newSettings.ConnectionMode) ||
+			!equalUint32Ptr(oldSettings.RelayTimeoutSeconds, newSettings.RelayTimeoutSeconds) ||
+			!equalUint32Ptr(oldSettings.P2pTimeoutSeconds, newSettings.P2pTimeoutSeconds) ||
+			!equalUint32Ptr(oldSettings.P2pRetryMaxSeconds, newSettings.P2pRetryMaxSeconds) ||
 			oldSettings.DNSDomain != newSettings.DNSDomain ||
 			oldSettings.AutoUpdateVersion != newSettings.AutoUpdateVersion ||
 			oldSettings.AutoUpdateAlways != newSettings.AutoUpdateAlways {
 			updateAccountPeers = true
 		}

🤖 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 `@management/server/account.go` around lines 372 - 375, The change handlers
currently only audit connection-mode related updates but do not push new runtime
config to live peers; update the caller to trigger am.updateAccountPeers when
any of ConnectionMode, RelayTimeoutSeconds, P2pTimeoutSeconds, or
P2pRetryMaxSeconds change. Concretely: modify handleConnectionModeSettings (or
have the handlers return a bool) so the outer code that calls
am.handleConnectionModeSettings(...) can detect a change and then call
am.updateAccountPeers(ctx, userID, accountID) (or the existing
updateAccountPeers signature) after the settings are saved/audited; ensure the
check compares oldSettings.ConnectionMode, oldSettings.RelayTimeoutSeconds,
oldSettings.P2pTimeoutSeconds, and oldSettings.P2pRetryMaxSeconds against the
newSettings versions and only calls updateAccountPeers when any differ.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reject negative timeout values before converting them to uint32.

These casts accept negative JSON values and wrap them into huge unsigned numbers, so a request like -1 becomes 4294967295 seconds instead of a validation error. Please validate all three timeout fields as >= 0 before the cast.

🛠️ Suggested fix

 	if req.Settings.P2pTimeoutSeconds != nil {
+		if *req.Settings.P2pTimeoutSeconds < 0 {
+			return nil, status.Errorf(status.InvalidArgument, "p2p_timeout_seconds must be >= 0")
+		}
 		v := uint32(*req.Settings.P2pTimeoutSeconds)
 		returnSettings.P2pTimeoutSeconds = &v
 	}
 	if req.Settings.P2pRetryMaxSeconds != nil {
+		if *req.Settings.P2pRetryMaxSeconds < 0 {
+			return nil, status.Errorf(status.InvalidArgument, "p2p_retry_max_seconds must be >= 0")
+		}
 		v := uint32(*req.Settings.P2pRetryMaxSeconds)
 		returnSettings.P2pRetryMaxSeconds = &v
 	}
 	if req.Settings.RelayTimeoutSeconds != nil {
+		if *req.Settings.RelayTimeoutSeconds < 0 {
+			return nil, status.Errorf(status.InvalidArgument, "relay_timeout_seconds must be >= 0")
+		}
 		v := uint32(*req.Settings.RelayTimeoutSeconds)
 		returnSettings.RelayTimeoutSeconds = &v
 	}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if req.Settings.P2pTimeoutSeconds != nil {
	v := uint32(*req.Settings.P2pTimeoutSeconds)
	returnSettings.P2pTimeoutSeconds = &v
	}
	if req.Settings.P2pRetryMaxSeconds != nil {
	v := uint32(*req.Settings.P2pRetryMaxSeconds)
	returnSettings.P2pRetryMaxSeconds = &v
	}
	if req.Settings.RelayTimeoutSeconds != nil {
	v := uint32(*req.Settings.RelayTimeoutSeconds)
	returnSettings.RelayTimeoutSeconds = &v
	}
	if req.Settings.P2pTimeoutSeconds != nil {
	if *req.Settings.P2pTimeoutSeconds < 0 {
	return nil, status.Errorf(status.InvalidArgument, "p2p_timeout_seconds must be >= 0")
	}
	v := uint32(*req.Settings.P2pTimeoutSeconds)
	returnSettings.P2pTimeoutSeconds = &v
	}
	if req.Settings.P2pRetryMaxSeconds != nil {
	if *req.Settings.P2pRetryMaxSeconds < 0 {
	return nil, status.Errorf(status.InvalidArgument, "p2p_retry_max_seconds must be >= 0")
	}
	v := uint32(*req.Settings.P2pRetryMaxSeconds)
	returnSettings.P2pRetryMaxSeconds = &v
	}
	if req.Settings.RelayTimeoutSeconds != nil {
	if *req.Settings.RelayTimeoutSeconds < 0 {
	return nil, status.Errorf(status.InvalidArgument, "relay_timeout_seconds must be >= 0")
	}
	v := uint32(*req.Settings.RelayTimeoutSeconds)
	returnSettings.RelayTimeoutSeconds = &v
	}

🤖 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 `@management/server/http/handlers/accounts/accounts_handler.go` around lines
229 - 240, The three timeout fields (req.Settings.P2pTimeoutSeconds,
P2pRetryMaxSeconds, RelayTimeoutSeconds) are being cast directly to uint32 which
allows negative JSON numbers to wrap to large unsigned values; update the
validation before casting in the handler that populates returnSettings: check
each pointer is non-nil and *value >= 0, and if a negative value is detected
return a validation/error response (or handle it per existing validation flow)
instead of casting; then safely convert to uint32 (e.g., v :=
uint32(*req.Settings.P2pTimeoutSeconds)) and assign to
returnSettings.P2pTimeoutSeconds, P2pRetryMaxSeconds, and RelayTimeoutSeconds
respectively.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update stale connection_mode phase semantics in the description.

This text says p2p-dynamic is reserved/pass-through, which conflicts with the current phase scope and can mislead API consumers.

✏️ Suggested doc update

         connection_mode:
           x-experimental: true
           type: string
           enum: [relay-forced, p2p, p2p-lazy, p2p-dynamic]
           nullable: true
           description: |
             Account-wide default peer-connection mode. NULL means
             "fall back to lazy_connection_enabled" for backwards compatibility.
-            Phase 1 of issue `#5989`: relay-forced, p2p, and p2p-lazy are
-            functional. p2p-dynamic is reserved (passes through as p2p in
-            Phase 1; will become functional in Phase 2).
+            relay-forced, p2p, p2p-lazy, and p2p-dynamic are supported.

🤖 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 `@shared/management/http/api/openapi.yml` around lines 367 - 372, The
description for the "connection_mode" enum is outdated and still states that
"p2p-dynamic" is reserved/passes through; update the OpenAPI description for the
connection_mode field so it reflects the current phase behavior (remove the
claim that p2p-dynamic is reserved/pass-through) and clearly document which
modes are currently functional (relay-forced, p2p, p2p-lazy) and how NULL
interacts with lazy_connection_enabled for backwards compatibility; edit the
description text near the connection_mode property in
shared/management/http/api/openapi.yml and mention the accurate phase semantics
for p2p-dynamic (omit or mark as not supported rather than pass-through) and
ensure references to lazy_connection_enabled and Phase 1/Phase 2 are consistent.

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (3)

client/internal/peer/worker_ice.go (1)

218-231: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

lastKnownState is still accessed without consistent synchronization — data race.

IsConnected() reads lastKnownState while holding muxAgent, but the writes inside onConnectionStateChange at lines 550 and 562 happen on the pion callback goroutine without acquiring muxAgent. This is a Go data race, and the new Conn.onNetworkChange path (in conn.go at line 1244) now reads through IsConnected() to decide whether to close the agent — a torn read can either skip Close() on a freshly-disconnected agent or close a still-Connected one, exactly the scenario the comment in onNetworkChange says to avoid.

Switch lastKnownState to atomic.Int32 (or similar) so reads and writes are consistent regardless of muxAgent:

-	// we record the last known state of the ICE agent to avoid duplicate on disconnected events
-	lastKnownState ice.ConnectionState
+	// Accessed from pion's state-change callback and from IsConnected()
+	// concurrently — keep atomic so reads on the network-change path are
+	// not torn against unlocked writes in onConnectionStateChange.
+	lastKnownState atomic.Int32 // ice.ConnectionState

-		lastKnownState:    ice.ConnectionStateDisconnected,
+	}
+	w.lastKnownState.Store(int32(ice.ConnectionStateDisconnected))

 func (w *WorkerICE) IsConnected() bool {
 	w.muxAgent.Lock()
 	defer w.muxAgent.Unlock()
-	return w.agent != nil && w.lastKnownState == ice.ConnectionStateConnected
+	return w.agent != nil && ice.ConnectionState(w.lastKnownState.Load()) == ice.ConnectionStateConnected
 }

 		case ice.ConnectionStateConnected:
-			w.lastKnownState = ice.ConnectionStateConnected
+			w.lastKnownState.Store(int32(ice.ConnectionStateConnected))
 			w.logSuccessfulPaths(agent)
@@
-			if w.lastKnownState == ice.ConnectionStateConnected {
-				w.lastKnownState = ice.ConnectionStateDisconnected
+			if ice.ConnectionState(w.lastKnownState.Load()) == ice.ConnectionStateConnected {
+				w.lastKnownState.Store(int32(ice.ConnectionStateDisconnected))
 				w.conn.onICEStateDisconnected(sessionChanged)
 			}

Also applies to: 549-563

🤖 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 `@client/internal/peer/worker_ice.go` around lines 218 - 231, IsConnected reads
lastKnownState under muxAgent but onConnectionStateChange writes it without
locking, causing a data race; change the WorkerICE.lastKnownState from a plain
int to an atomic.Int32 (or similar), update IsConnected() to use
lastKnownState.Load() (while keeping muxAgent for agent nil-check), and update
onConnectionStateChange to use lastKnownState.Store(newState) instead of
assigning directly; ensure all other accesses (e.g., in the code paths around
lines referenced) use atomic Load/Store and remove any unsynchronized direct
reads/writes to lastKnownState while leaving agent access protected by muxAgent
as before.

client/internal/peer/conn.go (1)

202-212: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Disable-backoff state is still not preserved across Open/Close cycles.

The ConnConfig comment at lines 98-100 says ^uint32(0) is the disable sentinel and the resolver translates it to 0 before reaching here, while the Open() block treats 0 as "use built-in default" (DefaultP2PRetryMax). These two semantics conflict: a peer that the resolver wanted disabled ends up with the default 15 m backoff. Likewise, SetIceBackoffMax(0) writes 0 into config.P2pRetryMaxSeconds, and a subsequent Close()/Open() cycle silently re-promotes the peer from "disabled" to the default schedule.

Use a distinct in-process representation for "disabled" so the three states (use default, disabled, explicit value) can each round-trip. For example, treat the wire ^uint32(0) as the in-process "disabled" sentinel rather than collapsing it to 0 at the resolver, so Open() can branch on it directly.

+const p2pRetryMaxDisabled = ^uint32(0)
+
-	backoffCap := time.Duration(conn.config.P2pRetryMaxSeconds) * time.Second
-	if backoffCap == 0 {
-		backoffCap = DefaultP2PRetryMax
-	}
+	var backoffCap time.Duration
+	switch conn.config.P2pRetryMaxSeconds {
+	case 0:
+		backoffCap = DefaultP2PRetryMax
+	case p2pRetryMaxDisabled:
+		backoffCap = 0
+	default:
+		backoffCap = time.Duration(conn.config.P2pRetryMaxSeconds) * time.Second
+	}
@@
 func (conn *Conn) SetIceBackoffMax(d time.Duration) {
 	conn.mu.Lock()
 	defer conn.mu.Unlock()
-	conn.config.P2pRetryMaxSeconds = uint32(d / time.Second)
+	if d == 0 {
+		conn.config.P2pRetryMaxSeconds = p2pRetryMaxDisabled
+	} else {
+		conn.config.P2pRetryMaxSeconds = uint32(d / time.Second)
+	}
 	if conn.iceBackoff != nil {
 		conn.iceBackoff.SetMaxBackoff(d)
 	}
 }

The companion change in the resolver/engine.go must also stop translating ^uint32(0) → 0 — pass the sentinel through verbatim.

Also applies to: 1152-1163

🤖 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 `@client/internal/peer/conn.go` around lines 202 - 212, The code collapses the
wire sentinel for "disabled" into 0 which conflicts with Open() treating 0 as
"use default", causing disabled peers to be re-enabled; fix by introducing a
distinct in-process sentinel for "disabled" (e.g. a named constant like
DisabledP2pRetrySentinel) and preserve the resolver wire sentinel instead of
translating it to 0 in resolver/engine.go, then update Open()'s logic around
conn.config.P2pRetryMaxSeconds to: if it equals the disabled sentinel, configure
conn.iceBackoff as disabled (or leave nil) so backoff is turned off; else if it
equals 0 treat it as "use default" and set backoffCap = DefaultP2PRetryMax,
otherwise use the explicit value to newIceBackoff / SetMaxBackoff; also ensure
SetIceBackoffMax writes the same in-process sentinel when disabling so
Close()/Open() round-trips.

client/android/preferences.go (1)

349-352: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Negative secs is silently aliased to a huge uint32; for SetP2pRetryMaxSeconds it collides with the disable sentinel.

A SetRelayTimeoutSeconds(-1) becomes 4294967295 seconds (~136 years), and the same bad cast on SetP2pRetryMaxSeconds(-1) yields exactly ^uint32(0), which is the documented wire-format sentinel for "user-explicit disable backoff" (see client/internal/peer/conn.go lines 98-100). Negative input therefore silently flips backoff into the disabled state without any validation. Clamp or reject < 0 in all three setters.

 func (p *Preferences) SetRelayTimeoutSeconds(secs int64) {
+	if secs < 0 {
+		secs = 0
+	}
 	v := uint32(secs)
 	p.configInput.RelayTimeoutSeconds = &v
 }
@@
 func (p *Preferences) SetP2pTimeoutSeconds(secs int64) {
+	if secs < 0 {
+		secs = 0
+	}
 	v := uint32(secs)
 	p.configInput.P2pTimeoutSeconds = &v
 }
@@
 func (p *Preferences) SetP2pRetryMaxSeconds(secs int64) {
+	if secs < 0 {
+		secs = 0
+	}
 	v := uint32(secs)
 	p.configInput.P2pRetryMaxSeconds = &v
 }

Also applies to: 370-373, 390-393

🤖 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 `@client/android/preferences.go` around lines 349 - 352, SetRelayTimeoutSeconds
(and the two other setters that cast int64 to uint32, e.g.,
SetP2pRetryMaxSeconds and the comparable setter) currently cast negative secs
into a huge uint32 (which can collide with the disable sentinel); update each
setter to validate secs before casting: if secs < 0 then clamp to 0 (or return
an error if you prefer API change) and only then assign via uint32(secs) into
the pointer field so negative inputs cannot become ^uint32(0) or other
unintended values.

🧹 Nitpick comments (6)

client/internal/peer/guard/guard.go (1)

57-62: ⚡ Quick win

SetOnNetworkChange after Start() is an undetected data race

The comment says "Set once before Start() is called; no lock needed." This is safe only if the goroutine spawning Start() happens strictly after SetOnNetworkChange returns, establishing a happens-before edge. However, nothing in the API enforces this — a caller that invokes SetOnNetworkChange after spawning go g.Start(...) introduces a data race on g.onNetworkChange with no diagnostic. Consider using sync/atomic to store the function pointer, or document this precondition with a //nolint:gocritic + go vet note so it surfaces in race-detector runs.

As a lightweight alternative, a sync/atomic.Pointer[func()] store/load pair makes the intent explicit and race-detector-clean with minimal overhead.

🤖 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 `@client/internal/peer/guard/guard.go` around lines 57 - 62, SetOnNetworkChange
currently writes g.onNetworkChange without synchronization which can race with
concurrent Start() readers; change the Guard implementation to store the
callback via a sync/atomic pointer type (e.g. sync/atomic.Pointer[func()])
instead of the plain field and update all reads (e.g. in Start and any other
goroutines that invoke the callback) to load via atomic.Load/Pointer (or the
generic Atomic.Pointer API) before invoking; ensure SetOnNetworkChange uses
atomic.Store/Pointer to set the callback and nil-checks use the atomic load so
access is race-detector-clean and retains the same semantics.

client/internal/peer/ice_backoff.go (2)

82-92: 💤 Low value

Optional: clear suspended flag on lazy expiry to keep state self-consistent.

IsSuspended() returns false once nextRetry is in the past, but it leaves s.suspended = true. Any future caller that inspects the raw flag (or that adds non-time-based logic) will see a stale "suspended" state. The condition is duplicated in Snapshot() (line 131) and the two helpers can drift. Consider clearing the flag once it has lapsed — pure read API can become a write (still safe under s.mu), or simply expose only the time-based predicate.

 func (s *iceBackoffState) IsSuspended() bool {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 	if !s.suspended {
 		return false
 	}
-	if time.Now().After(s.nextRetry) {
-		return false
-	}
-	return true
+	if time.Now().After(s.nextRetry) {
+		s.suspended = false
+		return false
+	}
+	return true
 }

🤖 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 `@client/internal/peer/ice_backoff.go` around lines 82 - 92, IsSuspended()
currently returns false when nextRetry has passed but leaves s.suspended true,
causing inconsistent state with Snapshot(); update IsSuspended() (in
iceBackoffState) to, under s.mu lock, check if s.suspended &&
time.Now().After(s.nextRetry) and if so clear s.suspended (and optionally reset
nextRetry) before returning false so the in-memory flags remain self-consistent;
ensure you modify only inside the locked section to avoid races and keep
Snapshot() behavior aligned.

---

161-169: 💤 Low value

Confirm: SetMaxBackoff resets the in-flight exponential schedule.

The doc comment states the failure counter is preserved, which it is, but buildBackoff calls bo.Reset() so the schedule restarts at InitialInterval. A peer that has climbed to e.g. ~8 m suspends will, after a server-pushed SetMaxBackoff to a different cap, drop back to ~1 m on the next failure. This is most likely intentional (new policy from server ⇒ fresh schedule), but the comment "preserves the failure counter" can be misread as "preserves the schedule"; consider clarifying.

-// SetMaxBackoff updates the cap. Called from ConnMgr.UpdatedRemotePeerConfig
-// when the server pushes a new value. Rebuilds the internal backoff with
-// the new schedule but preserves the failure counter.
+// SetMaxBackoff updates the cap. Called from ConnMgr.UpdatedRemotePeerConfig
+// when the server pushes a new value. Rebuilds the internal exponential
+// schedule from scratch (so the next markFailure starts at InitialInterval)
+// but preserves the cumulative failure counter for status reporting.

🤖 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 `@client/internal/peer/ice_backoff.go` around lines 161 - 169, Update the
comment for SetMaxBackoff on iceBackoffState to clarify that although the
failure counter is preserved, the exponential backoff schedule is reset because
SetMaxBackoff calls buildBackoff(d) which invokes bo.Reset(); explicitly state
that the schedule (current interval) restarts at InitialInterval while the
failure count remains unchanged so callers won't be misled into thinking the
in-flight schedule is preserved.

client/internal/peer/conn.go (2)

1030-1056: 💤 Low value

Minor: redundant IsSuspended() check.

AttachICE checks iceBackoff.IsSuspended() (line 1034) and then calls attachICEListenerLocked, which performs the same check (line 1070). The first check returns early without an error; the second returns false (no-op). The behaviour is identical, so the first block is just an extra log path that fires before the listener attach. If the dual logging is intentional (different message), keep it; otherwise drop the outer check and rely on attachICEListenerLocked returning false to skip the SendOffer.

 func (conn *Conn) AttachICE() error {
 	conn.mu.Lock()
 	defer conn.mu.Unlock()
 
-	if conn.iceBackoff != nil && conn.iceBackoff.IsSuspended() {
-		snap := conn.iceBackoff.Snapshot()
-		conn.Log.Debugf("ICE backoff active (failure #%d, retry at %s), staying on relay",
-			snap.Failures,
-			snap.NextRetry.Format("15:04:05"))
-		return nil
-	}
 	if conn.handshaker == nil {
 		return fmt.Errorf("AttachICE: handshaker not initialized (Open not called)")
 	}
 	if conn.workerICE == nil {
 		return fmt.Errorf("AttachICE: workerICE is nil (relay-forced mode)")
 	}
 
 	if !conn.attachICEListenerLocked() {
 		return nil
 	}

🤖 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 `@client/internal/peer/conn.go` around lines 1030 - 1056, Remove the redundant
iceBackoff.IsSuspended() check and its logging from Conn.AttachICE and rely on
attachICEListenerLocked to handle suspended backoff (so AttachICE no longer
early-returns for suspension); specifically, delete the block that checks
conn.iceBackoff != nil && conn.iceBackoff.IsSuspended() and its
conn.Log.Debugf/return, leaving the rest of AttachICE (including the handshaker
nil checks and the call to attachICEListenerLocked and SendOffer) intact so
behavior is unchanged except for avoiding duplicate suspension logging.

---

19-20: 💤 Low value

Import group ordering.

shared/connectionmode is wedged between client/internal/metrics and client/internal/peer/conntype, breaking the client/... group's alphabetical ordering. goimports would put shared/... after the client/... group. Minor, but easy to fix.

 	"github.com/netbirdio/netbird/client/internal/metrics"
-	"github.com/netbirdio/netbird/shared/connectionmode"
 	"github.com/netbirdio/netbird/client/internal/peer/conntype"
 	"github.com/netbirdio/netbird/client/internal/peer/dispatcher"
 	"github.com/netbirdio/netbird/client/internal/peer/guard"
 	icemaker "github.com/netbirdio/netbird/client/internal/peer/ice"
 	"github.com/netbirdio/netbird/client/internal/peer/id"
 	"github.com/netbirdio/netbird/client/internal/peer/worker"
 	"github.com/netbirdio/netbird/client/internal/portforward"
 	"github.com/netbirdio/netbird/client/internal/stdnet"
 	"github.com/netbirdio/netbird/route"
 	relayClient "github.com/netbirdio/netbird/shared/relay/client"
+	"github.com/netbirdio/netbird/shared/connectionmode"

🤖 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 `@client/internal/peer/conn.go` around lines 19 - 20, The import block in
client/internal/peer/conn.go has shared/connectionmode placed between
client/internal packages, breaking the client/... group's alphabetical grouping;
reorder the imports so all client/internal/* imports (e.g.,
client/internal/metrics and client/internal/peer/conntype) are contiguous and
alphabetically ordered, and move
github.com/netbirdio/netbird/shared/connectionmode after the client/... group
(matching goimports grouping).

client/internal/peer/env_test.go (1)

33-34: 💤 Low value

Use constants instead of hardcoded environment variable names.

Lines 31–32 already reference EnvKeyNBConnectionMode and EnvKeyNBForceRelay. Lines 33–34 hardcode "NB_ENABLE_EXPERIMENTAL_LAZY_CONN" and "NB_LAZY_CONN_INACTIVITY_THRESHOLD" as string literals.

The peer package defines these as private constants envEnableLazyConn and envInactivityThreshold (accessible within the same package), or import EnvEnableLazyConn and EnvInactivityThreshold from client/internal/lazyconn. Using constants ensures any rename is caught at compile time.

🤖 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 `@client/internal/peer/env_test.go` around lines 33 - 34, Replace the hardcoded
environment variable names in the test t.Setenv calls with the package
constants: use envEnableLazyConn instead of "NB_ENABLE_EXPERIMENTAL_LAZY_CONN"
and envInactivityThreshold instead of "NB_LAZY_CONN_INACTIVITY_THRESHOLD" (these
constants are defined in the peer package); update the two t.Setenv calls in
env_test.go to reference those constants so renames are caught at compile time.

🤖 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 `@client/android/preferences.go`:
- Around line 325-352: The setters store pointers to zero values instead of
clearing overrides, so change SetConnectionMode, SetRelayTimeoutSeconds (and
likewise SetP2pTimeoutSeconds and SetP2pRetryMaxSeconds) to set the
corresponding pointer to nil when the sentinel is passed (empty string for
SetConnectionMode, 0 for the timeout/setter methods); otherwise allocate and
assign a pointer to the non-zero value. Update p.configInput.ConnectionMode and
p.configInput.RelayTimeoutSeconds (and the P2P fields) accordingly so apply()'s
nil check correctly reverts to server-pushed values.

In `@client/internal/peer/guard/guard.go`:
- Around line 144-147: The select loop currently calls g.onNetworkChange()
synchronously which blocks responsiveness to ctx.Done() and other channels;
change it to dispatch the callback asynchronously: when g.onNetworkChange != nil
spawn a background goroutine to call g.onNetworkChange() from the guard's select
branch (e.g., in the guard loop where srReconnectedChan is handled) so the loop
can continue processing ctx.Done and other events. To avoid unbounded concurrent
invocations, add a simple coalescing mechanism (a boolean flag or non-blocking
send to a buffered channel) adjacent to the async dispatch to ensure rapid
reconnect events don’t spawn redundant goroutines unless the previous handler
has completed; keep the flag/chan state local to the guard instance that owns
g.onNetworkChange.

In `@client/proto/daemon.proto`:
- Around line 208-218: The daemon handlers in client/server/server.go currently
ignore the new LoginRequest.connection_mode, p2p_timeout_seconds,
relay_timeout_seconds, and p2p_retry_max_seconds (and the same fields on
SetConfigRequest), so RPC callers can send them but the daemon neither applies
nor persists them; update the request handling code paths that process
LoginRequest and SetConfigRequest in server.go to read these fields,
validate/parse connection_mode (using connectionmode.ParseString where
appropriate), apply them to the daemon's runtime config/state, and persist them
to the same storage path used by existing config fields so the GUI save flow and
subsequent calls reflect the overrides. Ensure the same wiring is added for any
helper methods that currently consume LoginRequest or SetConfigRequest (so both
request handling and persistence are consistent).

---

Duplicate comments:
In `@client/android/preferences.go`:
- Around line 349-352: SetRelayTimeoutSeconds (and the two other setters that
cast int64 to uint32, e.g., SetP2pRetryMaxSeconds and the comparable setter)
currently cast negative secs into a huge uint32 (which can collide with the
disable sentinel); update each setter to validate secs before casting: if secs <
0 then clamp to 0 (or return an error if you prefer API change) and only then
assign via uint32(secs) into the pointer field so negative inputs cannot become
^uint32(0) or other unintended values.

In `@client/internal/peer/conn.go`:
- Around line 202-212: The code collapses the wire sentinel for "disabled" into
0 which conflicts with Open() treating 0 as "use default", causing disabled
peers to be re-enabled; fix by introducing a distinct in-process sentinel for
"disabled" (e.g. a named constant like DisabledP2pRetrySentinel) and preserve
the resolver wire sentinel instead of translating it to 0 in resolver/engine.go,
then update Open()'s logic around conn.config.P2pRetryMaxSeconds to: if it
equals the disabled sentinel, configure conn.iceBackoff as disabled (or leave
nil) so backoff is turned off; else if it equals 0 treat it as "use default" and
set backoffCap = DefaultP2PRetryMax, otherwise use the explicit value to
newIceBackoff / SetMaxBackoff; also ensure SetIceBackoffMax writes the same
in-process sentinel when disabling so Close()/Open() round-trips.

In `@client/internal/peer/worker_ice.go`:
- Around line 218-231: IsConnected reads lastKnownState under muxAgent but
onConnectionStateChange writes it without locking, causing a data race; change
the WorkerICE.lastKnownState from a plain int to an atomic.Int32 (or similar),
update IsConnected() to use lastKnownState.Load() (while keeping muxAgent for
agent nil-check), and update onConnectionStateChange to use
lastKnownState.Store(newState) instead of assigning directly; ensure all other
accesses (e.g., in the code paths around lines referenced) use atomic Load/Store
and remove any unsynchronized direct reads/writes to lastKnownState while
leaving agent access protected by muxAgent as before.

---

Nitpick comments:
In `@client/internal/peer/conn.go`:
- Around line 1030-1056: Remove the redundant iceBackoff.IsSuspended() check and
its logging from Conn.AttachICE and rely on attachICEListenerLocked to handle
suspended backoff (so AttachICE no longer early-returns for suspension);
specifically, delete the block that checks conn.iceBackoff != nil &&
conn.iceBackoff.IsSuspended() and its conn.Log.Debugf/return, leaving the rest
of AttachICE (including the handshaker nil checks and the call to
attachICEListenerLocked and SendOffer) intact so behavior is unchanged except
for avoiding duplicate suspension logging.
- Around line 19-20: The import block in client/internal/peer/conn.go has
shared/connectionmode placed between client/internal packages, breaking the
client/... group's alphabetical grouping; reorder the imports so all
client/internal/* imports (e.g., client/internal/metrics and
client/internal/peer/conntype) are contiguous and alphabetically ordered, and
move github.com/netbirdio/netbird/shared/connectionmode after the client/...
group (matching goimports grouping).

In `@client/internal/peer/env_test.go`:
- Around line 33-34: Replace the hardcoded environment variable names in the
test t.Setenv calls with the package constants: use envEnableLazyConn instead of
"NB_ENABLE_EXPERIMENTAL_LAZY_CONN" and envInactivityThreshold instead of
"NB_LAZY_CONN_INACTIVITY_THRESHOLD" (these constants are defined in the peer
package); update the two t.Setenv calls in env_test.go to reference those
constants so renames are caught at compile time.

In `@client/internal/peer/guard/guard.go`:
- Around line 57-62: SetOnNetworkChange currently writes g.onNetworkChange
without synchronization which can race with concurrent Start() readers; change
the Guard implementation to store the callback via a sync/atomic pointer type
(e.g. sync/atomic.Pointer[func()]) instead of the plain field and update all
reads (e.g. in Start and any other goroutines that invoke the callback) to load
via atomic.Load/Pointer (or the generic Atomic.Pointer API) before invoking;
ensure SetOnNetworkChange uses atomic.Store/Pointer to set the callback and
nil-checks use the atomic load so access is race-detector-clean and retains the
same semantics.

In `@client/internal/peer/ice_backoff.go`:
- Around line 82-92: IsSuspended() currently returns false when nextRetry has
passed but leaves s.suspended true, causing inconsistent state with Snapshot();
update IsSuspended() (in iceBackoffState) to, under s.mu lock, check if
s.suspended && time.Now().After(s.nextRetry) and if so clear s.suspended (and
optionally reset nextRetry) before returning false so the in-memory flags remain
self-consistent; ensure you modify only inside the locked section to avoid races
and keep Snapshot() behavior aligned.
- Around line 161-169: Update the comment for SetMaxBackoff on iceBackoffState
to clarify that although the failure counter is preserved, the exponential
backoff schedule is reset because SetMaxBackoff calls buildBackoff(d) which
invokes bo.Reset(); explicitly state that the schedule (current interval)
restarts at InitialInterval while the failure count remains unchanged so callers
won't be misled into thinking the in-flight schedule is preserved.

🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d4622b57-deca-4419-b794-100392cfb3c4

📥 Commits

Reviewing files that changed from the base of the PR and between 2b6f78b and 6bf6777.

⛔ Files ignored due to path filters (2)

• client/proto/daemon.pb.go is excluded by !**/*.pb.go
• shared/management/proto/management.pb.go is excluded by !**/*.pb.go

📒 Files selected for processing (20)

• client/android/client.go
• client/android/preferences.go
• client/cmd/service.go
• client/cmd/service_installer.go
• client/internal/conn_mgr.go
• client/internal/conn_mgr_test.go
• client/internal/engine.go
• client/internal/peer/conn.go
• client/internal/peer/env_test.go
• client/internal/peer/guard/guard.go
• client/internal/peer/handshaker.go
• client/internal/peer/ice_backoff.go
• client/internal/peer/ice_backoff_test.go
• client/internal/peer/worker_ice.go
• client/proto/daemon.proto
• client/server/server.go
• client/server/setconfig_test.go
• client/ui/client_ui.go
• client/ui/const.go
• client/ui/event_handler.go

💤 Files with no reviewable changes (2)

• client/ui/event_handler.go
• client/ui/const.go

🚧 Files skipped from review as they are similar to previous changes (1)

• client/internal/conn_mgr.go

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm there's no other "clear override" path that nils the pointer in apply() or elsewhere.
rg -nP --type=go -C2 'ConnectionMode|RelayTimeoutSeconds|P2pTimeoutSeconds|P2pRetryMaxSeconds' -g '!**/*_test.go' -g 'client/internal/profilemanager/**'

Repository: netbirdio/netbird

Length of output: 3375

---

🏁 Script executed:

#!/bin/bash
# Check the actual SetConnectionMode, SetRelayTimeoutSeconds, SetP2pTimeoutSeconds, SetP2pRetryMaxSeconds implementations
cat -n client/android/preferences.go | head -400 | tail -100

Repository: netbirdio/netbird

Length of output: 4108

---

🏁 Script executed:

#!/bin/bash
# Search for all occurrences of these four setter functions to ensure we catch all of them
rg -n 'func.*Set(ConnectionMode|RelayTimeoutSeconds|P2pTimeoutSeconds|P2pRetryMaxSeconds)' --type=go -g '!**/*_test.go'

Repository: netbirdio/netbird

Length of output: 426

---

🏁 Script executed:

#!/bin/bash
# Check if there are any other code paths that might clear these overrides (e.g., by setting to nil)
rg -n 'ConnectionMode.*=.*nil|RelayTimeoutSeconds.*=.*nil|P2pTimeoutSeconds.*=.*nil|P2pRetryMaxSeconds.*=.*nil' --type=go -g '!**/*_test.go'

Repository: netbirdio/netbird

Length of output: 3062

---

Fix the "clear override" logic to set pointer to nil, not to zero value.

The documented behaviour for SetConnectionMode, SetRelayTimeoutSeconds, SetP2pTimeoutSeconds, and SetP2pRetryMaxSeconds promises that passing 0 (or empty string) clears the override and reverts to the server-pushed value. However, each setter currently stores a pointer to the zero value, and the apply() function in client/internal/profilemanager/config.go (lines 608–626) checks only != nil before assigning. This overwrites the persisted server-pushed value with 0 or "" instead of clearing the override.

To honour the documented semantics, set the pointer to nil when the sentinel value is passed:

Suggested fix pattern

 func (p *Preferences) SetConnectionMode(mode string) {
+	if mode == "" {
+		p.configInput.ConnectionMode = nil
+		return
+	}
 	m := mode
 	p.configInput.ConnectionMode = &m
 }
@@
 func (p *Preferences) SetRelayTimeoutSeconds(secs int64) {
+	if secs <= 0 {
+		p.configInput.RelayTimeoutSeconds = nil
+		return
+	}
 	v := uint32(secs)
 	p.configInput.RelayTimeoutSeconds = &v
 }

Apply the same pattern to SetP2pTimeoutSeconds and SetP2pRetryMaxSeconds.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	// SetConnectionMode stores a local override for the connection mode.
	// Pass an empty string to clear the override (revert to following the
	// server-pushed value).
	func (p *Preferences) SetConnectionMode(mode string) {
	m := mode
	p.configInput.ConnectionMode = &m
	}
	// GetRelayTimeoutSeconds returns the locally configured relay-worker
	// inactivity timeout in seconds, or 0 if no override is set (follow
	// server-pushed value, or built-in default if the server has none).
	func (p *Preferences) GetRelayTimeoutSeconds() (int64, error) {
	if p.configInput.RelayTimeoutSeconds != nil {
	return int64(*p.configInput.RelayTimeoutSeconds), nil
	}
	cfg, err := profilemanager.ReadConfig(p.configInput.ConfigPath)
	if err != nil {
	return 0, err
	}
	return int64(cfg.RelayTimeoutSeconds), nil
	}
	// SetRelayTimeoutSeconds stores a local override for the relay timeout.
	// Pass 0 to clear the override.
	func (p *Preferences) SetRelayTimeoutSeconds(secs int64) {
	v := uint32(secs)
	p.configInput.RelayTimeoutSeconds = &v
	}
	// SetConnectionMode stores a local override for the connection mode.
	// Pass an empty string to clear the override (revert to following the
	// server-pushed value).
	func (p *Preferences) SetConnectionMode(mode string) {
	if mode == "" {
	p.configInput.ConnectionMode = nil
	return
	}
	m := mode
	p.configInput.ConnectionMode = &m
	}
	// GetRelayTimeoutSeconds returns the locally configured relay-worker
	// inactivity timeout in seconds, or 0 if no override is set (follow
	// server-pushed value, or built-in default if the server has none).
	func (p *Preferences) GetRelayTimeoutSeconds() (int64, error) {
	if p.configInput.RelayTimeoutSeconds != nil {
	return int64(*p.configInput.RelayTimeoutSeconds), nil
	}
	cfg, err := profilemanager.ReadConfig(p.configInput.ConfigPath)
	if err != nil {
	return 0, err
	}
	return int64(cfg.RelayTimeoutSeconds), nil
	}
	// SetRelayTimeoutSeconds stores a local override for the relay timeout.
	// Pass 0 to clear the override.
	func (p *Preferences) SetRelayTimeoutSeconds(secs int64) {
	if secs <= 0 {
	p.configInput.RelayTimeoutSeconds = nil
	return
	}
	v := uint32(secs)
	p.configInput.RelayTimeoutSeconds = &v
	}

🤖 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 `@client/android/preferences.go` around lines 325 - 352, The setters store
pointers to zero values instead of clearing overrides, so change
SetConnectionMode, SetRelayTimeoutSeconds (and likewise SetP2pTimeoutSeconds and
SetP2pRetryMaxSeconds) to set the corresponding pointer to nil when the sentinel
is passed (empty string for SetConnectionMode, 0 for the timeout/setter
methods); otherwise allocate and assign a pointer to the non-zero value. Update
p.configInput.ConnectionMode and p.configInput.RelayTimeoutSeconds (and the P2P
fields) accordingly so apply()'s nil check correctly reverts to server-pushed
values.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Synchronous onNetworkChange() call blocks ctx.Done() responsiveness

The callback is invoked on the guard's select goroutine. Per the PR description, it recreates the ICE worker — potentially involving non-trivial setup. Until the callback returns, the guard loop cannot react to ctx.Done() or any other channel (including another srReconnectedChan event). Consider dispatching it in a background goroutine:

⚡ Proposed fix

-            // Phase 3.5 (`#5989`): notify Conn to reset iceBackoff + recreate workerICE
-            if g.onNetworkChange != nil {
-                g.onNetworkChange()
-            }
+            // Phase 3.5 (`#5989`): notify Conn to reset iceBackoff + recreate workerICE.
+            // Dispatched asynchronously so the guard loop remains responsive to ctx.Done().
+            if g.onNetworkChange != nil {
+                go g.onNetworkChange()
+            }

If the callback itself serializes on an internal mutex (as ICE worker recreation likely does), concurrent calls from rapid reconnect events are already serialized. If not, consider adding a flag or draining mechanism to coalesce multiple rapid network-change signals.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	// Phase 3.5 (#5989): notify Conn to reset iceBackoff + recreate workerICE
	if g.onNetworkChange != nil {
	g.onNetworkChange()
	}
	// Phase 3.5 (`#5989`): notify Conn to reset iceBackoff + recreate workerICE.
	// Dispatched asynchronously so the guard loop remains responsive to ctx.Done().
	if g.onNetworkChange != nil {
	go g.onNetworkChange()
	}

🤖 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 `@client/internal/peer/guard/guard.go` around lines 144 - 147, The select loop
currently calls g.onNetworkChange() synchronously which blocks responsiveness to
ctx.Done() and other channels; change it to dispatch the callback
asynchronously: when g.onNetworkChange != nil spawn a background goroutine to
call g.onNetworkChange() from the guard's select branch (e.g., in the guard loop
where srReconnectedChan is handled) so the loop can continue processing ctx.Done
and other events. To avoid unbounded concurrent invocations, add a simple
coalescing mechanism (a boolean flag or non-blocking send to a buffered channel)
adjacent to the async dispatch to ensure rapid reconnect events don’t spawn
redundant goroutines unless the previous handler has completed; keep the
flag/chan state local to the guard instance that owns g.onNetworkChange.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Wire these new request fields through the daemon handlers.

client/server/server.go still ignores LoginRequest.connection_mode/... and SetConfigRequest.connection_mode/..., so callers can send these overrides and get a success response even though the daemon neither applies nor persists them. That leaves the new RPC surface out of sync with the implementation, especially for the GUI save flow.

Also applies to: 714-724

🤖 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 `@client/proto/daemon.proto` around lines 208 - 218, The daemon handlers in
client/server/server.go currently ignore the new LoginRequest.connection_mode,
p2p_timeout_seconds, relay_timeout_seconds, and p2p_retry_max_seconds (and the
same fields on SetConfigRequest), so RPC callers can send them but the daemon
neither applies nor persists them; update the request handling code paths that
process LoginRequest and SetConfigRequest in server.go to read these fields,
validate/parse connection_mode (using connectionmode.ParseString where
appropriate), apply them to the daemon's runtime config/state, and persist them
to the same storage path used by existing config fields so the GUI save flow and
subsequent calls reflect the overrides. Ensure the same wiring is added for any
helper methods that currently consume LoginRequest or SetConfigRequest (so both
request handling and persistence are consistent).

[sonarqubecloud]

Quality Gate failed

Failed conditions
7 New issues
7 New Code Smells (required ≤ 0)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (7)

client/internal/peer/guard/guard.go (1)

144-147: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Synchronous onNetworkChange() still blocks the guard select loop

The callback is still invoked synchronously inside the srReconnectedChan branch. Until it returns, ctx.Done(), relayedConnDisconnected, iCEConnDisconnected, and any further srReconnectedChan events are all blocked. This was flagged in the previous review and remains unresolved.

🛠 Proposed fix (async dispatch with single-flight coalescing)

+		// networkChangePending prevents redundant concurrent recreations.
+		var networkChangePending atomic.Bool

 		case <-srReconnectedChan:
 			g.log.Debugf("has network changes, reset reconnection ticker")
 			ticker.Stop()
 			ticker = g.newReconnectTicker(ctx)
 			tickerChannel = ticker.C
 			iceState.reset()
-			// Phase 3.5 (`#5989`): notify Conn to reset iceBackoff + recreate workerICE
-			if g.onNetworkChange != nil {
-				g.onNetworkChange()
-			}
+			// Phase 3.5 (`#5989`): notify Conn to reset iceBackoff + recreate workerICE.
+			// Dispatched asynchronously so the guard loop stays responsive to ctx.Done().
+			if g.onNetworkChange != nil && !networkChangePending.Swap(true) {
+				go func() {
+					defer networkChangePending.Store(false)
+					g.onNetworkChange()
+				}()
+			}

🤖 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 `@client/internal/peer/guard/guard.go` around lines 144 - 147, The
srReconnectedChan branch currently calls g.onNetworkChange() synchronously
inside the guard select loop, which blocks other cases; change it to dispatch
the work asynchronously: when handling srReconnectedChan, if g.onNetworkChange
!= nil spawn a goroutine to invoke it instead of calling it inline and use a
single-flight/coalescing mechanism (e.g., a singleflight.Group or an in-struct
boolean guarded by a mutex/atomic) so concurrent srReconnectedChan events only
trigger one inflight onNetworkChange invocation; ensure the async worker
performs the existing reset iceBackoff and recreate workerICE logic and that you
respect ctx cancellation to avoid leaking goroutines.

client/internal/peer/worker_ice.go (1)

218-230: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Synchronize lastKnownState across the callback and network-change paths.

IsConnected() at Lines 227-230 now drives the skip-close branch, but onConnectionStateChange() still writes lastKnownState without the same synchronization at Lines 550 and 562. That leaves a data race on the exact close-or-skip decision this PR added.

#!/bin/bash
rg -n -C2 '\blastKnownState\b' client/internal/peer/worker_ice.go

Also applies to: 545-563

🤖 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 `@client/internal/peer/worker_ice.go` around lines 218 - 230, IsConnected reads
lastKnownState under w.muxAgent while onConnectionStateChange writes
lastKnownState without locking, creating a race; fix by protecting all accesses
to lastKnownState with the same mutex (w.muxAgent) — update
onConnectionStateChange (the callback that sets lastKnownState at/around where
it currently assigns at lines ~545-563) to acquire w.muxAgent.Lock() and defer
w.muxAgent.Unlock() around writes (and any reads), ensuring symmetry with
IsConnected and preventing the close-or-skip race.

client/internal/conn_mgr.go (2)

282-320: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Apply timeout pushes even when the managed mode itself doesn’t change.

Lines 282-320 only restart the manager on modeChanged, but initLazyManager() consumes relayTimeoutSecs and p2pTimeoutSecs once at Lines 557-562. A push from, for example, p2p-dynamic(60/30) to p2p-dynamic(10/5) updates the fields but leaves the live manager running with the old thresholds until a full restart.

Also applies to: 553-562

🤖 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 `@client/internal/conn_mgr.go` around lines 282 - 320, The runtime ignores
timeout-only pushes if the mode doesn’t change; detect when
relayTimeoutSecs/p2pTimeoutSecs/p2pRetryMaxSecs change while a manager is active
and apply them by restarting or updating the manager: in conn_mgr.go where
modeChanged/wasManaged/isManaged are computed (and where lazyConnMgr is
checked), add a branch that if isManaged && e.lazyConnMgr != nil and any of
relayTimeoutSecs/p2pTimeoutSecs/p2pRetryMaxSecs differ from the manager’s
current thresholds, call e.closeManager(ctx) then e.initLazyManager(ctx),
e.startModeSideEffects(), and e.addPeersToLazyConnManager() (or alternatively
propagate the new values into the running manager); reference modeUsesLazyMgr,
lazyConnMgr, relayTimeoutSecs, p2pTimeoutSecs, p2pRetryMaxSecs,
propagateP2pRetryMaxToConns, initLazyManager, closeManager,
startModeSideEffects, and addPeersToLazyConnManager when making the change.

---

181-188: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don’t leave e.mode in lazy/dynamic when Rosenpass prevents the manager from starting.

Lines 185-187 and 313-315 return early, but e.mode is still kept/set to p2p-lazy or p2p-dynamic. New peers then inherit that mode, AddPeerConn() falls back to conn.Open() at Lines 391-395 because no manager exists, and ModeP2PDynamic still defers ICE listener registration in client/internal/peer/conn.go at Lines 239-242. Since ActivatePeer() also returns early with no manager at Lines 449-451, those peers can stay relay-only indefinitely.

Also applies to: 312-320

🤖 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 `@client/internal/conn_mgr.go` around lines 181 - 188, When you bail out from
starting the lazy/dynamic manager (both the modeUsesLazyMgr() check and the
e.rosenpassEnabled check), clear/unset the lazy P2P mode on e.mode so it no
longer advertises "p2p-lazy" or "p2p-dynamic" to new peers; specifically, before
each early return set e.mode to a non-lazy value (i.e. remove the lazy/dynamic
flag or assign the normal/direct P2P mode) so callers like AddPeerConn() and
ActivatePeer() won’t treat peers as manager-backed (avoiding the conn.Open()
fallback and deferred ICE registration in ModeP2PDynamic).

client/internal/peer/conn.go (1)

202-212: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Keep the explicit “disable backoff” state round-trippable.

propagateP2pRetryMaxToConns() converts the wire sentinel to time.Duration(0), but Line 1159 stores that back as plain 0. After a later reopen, Lines 204-207 reinterpret 0 as “use default”, so a user-explicit disable silently turns back into the 15-minute cap.

Also applies to: 1156-1162

🤖 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 `@client/internal/peer/conn.go` around lines 202 - 212, The explicit "disable
backoff" sentinel is being lost because 0 is being reused for "use default";
introduce and use a distinct sentinel (e.g. const P2PRetryMaxDisabledSeconds =
-1) and update both propagateP2pRetryMaxToConns() (the code around 1156-1162)
and this initialization block in conn.go to interpret -1 as "disabled" (set
backoffCap to 0 or disable the iceBackoff entirely) while treating 0 as "use
built-in default" (map to DefaultP2PRetryMax); also ensure
newIceBackoff()/iceBackoff.SetMaxBackoff accept/handle the disabled sentinel so
the explicit disable round-trips unchanged.

client/ui/client_ui.go (1)

656-666: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Do not silently coerce invalid timeout input to 0.

Invalid/overflow timeout text currently collapses to 0, which can unintentionally clear overrides when saving settings. Parse should return an error and buildSetConfigRequest should surface it.

Proposed fix

-func parseUint32Field(text string) uint32 {
+func parseUint32Field(text string) (uint32, error) {
 	t := strings.TrimSpace(text)
 	if t == "" {
-		return 0
+		return 0, nil
 	}
 	v, err := strconv.ParseUint(t, 10, 32)
 	if err != nil {
-		return 0
+		return 0, fmt.Errorf("must be a whole number between 0 and %d", uint64(^uint32(0)))
 	}
-	return uint32(v)
+	return uint32(v), nil
 }
@@
-	relaySecs := parseUint32Field(s.iRelayTimeout.Text)
-	p2pSecs := parseUint32Field(s.iP2pTimeout.Text)
-	retrySecs := parseUint32Field(s.iP2pRetryMax.Text)
+	relaySecs, err := parseUint32Field(s.iRelayTimeout.Text)
+	if err != nil {
+		return nil, fmt.Errorf("invalid relay timeout: %w", err)
+	}
+	p2pSecs, err := parseUint32Field(s.iP2pTimeout.Text)
+	if err != nil {
+		return nil, fmt.Errorf("invalid P2P timeout: %w", err)
+	}
+	retrySecs, err := parseUint32Field(s.iP2pRetryMax.Text)
+	if err != nil {
+		return nil, fmt.Errorf("invalid P2P retry-max: %w", err)
+	}
 	req.RelayTimeoutSeconds = &relaySecs
 	req.P2PTimeoutSeconds = &p2pSecs
 	req.P2PRetryMaxSeconds = &retrySecs

Also applies to: 741-750

🤖 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 `@client/ui/client_ui.go` around lines 656 - 666, The parseUint32Field function
currently swallows parse/overflow errors and returns 0 which can accidentally
clear saved timeouts; change parseUint32Field to return (uint32, error) instead
of silently coercing — validate strings.TrimSpace(text) then call
strconv.ParseUint(..., 32) and return an explicit error on failure (including
overflow), and update all callers (notably buildSetConfigRequest) to propagate
or surface that error to the caller (e.g., return/build an error response
instead of using 0); apply the same change to the other similar parser at the
741-750 block so all timeout parsing surfaces errors instead of returning 0.

client/android/preferences.go (1)

349-352: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard negative timeout inputs before casting to uint32.

Negative values currently wrap into very large uint32 values and can silently corrupt timeout overrides. Treat <= 0 as “clear override” (0) before casting.

Proposed fix

 func (p *Preferences) SetRelayTimeoutSeconds(secs int64) {
+	if secs <= 0 {
+		v := uint32(0)
+		p.configInput.RelayTimeoutSeconds = &v
+		return
+	}
 	v := uint32(secs)
 	p.configInput.RelayTimeoutSeconds = &v
 }
@@
 func (p *Preferences) SetP2pTimeoutSeconds(secs int64) {
+	if secs <= 0 {
+		v := uint32(0)
+		p.configInput.P2pTimeoutSeconds = &v
+		return
+	}
 	v := uint32(secs)
 	p.configInput.P2pTimeoutSeconds = &v
 }
@@
 func (p *Preferences) SetP2pRetryMaxSeconds(secs int64) {
+	if secs <= 0 {
+		v := uint32(0)
+		p.configInput.P2pRetryMaxSeconds = &v
+		return
+	}
 	v := uint32(secs)
 	p.configInput.P2pRetryMaxSeconds = &v
 }

Also applies to: 370-373, 390-393

🤖 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 `@client/android/preferences.go` around lines 349 - 352, Guard against negative
timeout inputs in SetRelayTimeoutSeconds: treat any secs <= 0 as a clear
override by using 0 instead of allowing a negative int64 to wrap when cast to
uint32. Update SetRelayTimeoutSeconds to check secs <= 0 and set the config
pointer to a uint32(0) in that case; otherwise cast secs to uint32 and assign as
before. Apply the same pattern to the other timeout setter functions in this
file that perform the uint32 cast so they also handle non-positive inputs
safely.

🧹 Nitpick comments (1)

client/internal/lazyconn/manager/manager.go (1)

68-68: ⚡ Quick win

Remove unused inactivityThreshold field from Manager struct.

The field is assigned in NewManager (line 93) but never read; all timeout logic flows through resolvedTimeouts(). It's dead code from the migration and can be safely removed.

🧹 Removal diff

 type Manager struct {
 	engineCtx           context.Context
 	peerStore           *peerstore.Store
-	inactivityThreshold time.Duration
 
 	managedPeers         map[string]*lazyconn.PeerConfig

 	m := &Manager{
 		engineCtx:            engineCtx,
 		peerStore:            peerStore,
-		inactivityThreshold:  inactivity.DefaultInactivityThreshold,
 		managedPeers:         make(map[string]*lazyconn.PeerConfig),

🤖 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 `@client/internal/lazyconn/manager/manager.go` at line 68, Remove the dead
inactivityThreshold field from the Manager struct and eliminate its assignment
in NewManager: delete the inactivityThreshold declaration in the Manager type,
remove any parameter/argument or line in NewManager that sets
m.inactivityThreshold, and ensure no other code reads it (timeout logic should
remain using resolvedTimeouts()). Update related constructors/call sites to stop
passing that value and run tests to confirm nothing else references
inactivityThreshold.

🤖 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 `@client/cmd/service_installer.go`:
- Around line 135-139: The call to applyConnectionModeFlagsToProfile currently
logs a warning on failure, allowing install/reconfigure to continue with
inconsistent connection-mode state; change this so that any error returned by
applyConnectionModeFlagsToProfile is propagated and causes the command to abort
(return the error) instead of printing a warning. Update both places where this
call appears (the install path around applyConnectionModeFlagsToProfile(cmd) and
the reconfigure path) to return the error up the call stack rather than using
cmd.PrintErrf, ensuring the command stops if persistence fails and no further
install/reconfigure steps run.

---

Duplicate comments:
In `@client/android/preferences.go`:
- Around line 349-352: Guard against negative timeout inputs in
SetRelayTimeoutSeconds: treat any secs <= 0 as a clear override by using 0
instead of allowing a negative int64 to wrap when cast to uint32. Update
SetRelayTimeoutSeconds to check secs <= 0 and set the config pointer to a
uint32(0) in that case; otherwise cast secs to uint32 and assign as before.
Apply the same pattern to the other timeout setter functions in this file that
perform the uint32 cast so they also handle non-positive inputs safely.

In `@client/internal/conn_mgr.go`:
- Around line 282-320: The runtime ignores timeout-only pushes if the mode
doesn’t change; detect when relayTimeoutSecs/p2pTimeoutSecs/p2pRetryMaxSecs
change while a manager is active and apply them by restarting or updating the
manager: in conn_mgr.go where modeChanged/wasManaged/isManaged are computed (and
where lazyConnMgr is checked), add a branch that if isManaged && e.lazyConnMgr
!= nil and any of relayTimeoutSecs/p2pTimeoutSecs/p2pRetryMaxSecs differ from
the manager’s current thresholds, call e.closeManager(ctx) then
e.initLazyManager(ctx), e.startModeSideEffects(), and
e.addPeersToLazyConnManager() (or alternatively propagate the new values into
the running manager); reference modeUsesLazyMgr, lazyConnMgr, relayTimeoutSecs,
p2pTimeoutSecs, p2pRetryMaxSecs, propagateP2pRetryMaxToConns, initLazyManager,
closeManager, startModeSideEffects, and addPeersToLazyConnManager when making
the change.
- Around line 181-188: When you bail out from starting the lazy/dynamic manager
(both the modeUsesLazyMgr() check and the e.rosenpassEnabled check), clear/unset
the lazy P2P mode on e.mode so it no longer advertises "p2p-lazy" or
"p2p-dynamic" to new peers; specifically, before each early return set e.mode to
a non-lazy value (i.e. remove the lazy/dynamic flag or assign the normal/direct
P2P mode) so callers like AddPeerConn() and ActivatePeer() won’t treat peers as
manager-backed (avoiding the conn.Open() fallback and deferred ICE registration
in ModeP2PDynamic).

In `@client/internal/peer/conn.go`:
- Around line 202-212: The explicit "disable backoff" sentinel is being lost
because 0 is being reused for "use default"; introduce and use a distinct
sentinel (e.g. const P2PRetryMaxDisabledSeconds = -1) and update both
propagateP2pRetryMaxToConns() (the code around 1156-1162) and this
initialization block in conn.go to interpret -1 as "disabled" (set backoffCap to
0 or disable the iceBackoff entirely) while treating 0 as "use built-in default"
(map to DefaultP2PRetryMax); also ensure
newIceBackoff()/iceBackoff.SetMaxBackoff accept/handle the disabled sentinel so
the explicit disable round-trips unchanged.

In `@client/internal/peer/guard/guard.go`:
- Around line 144-147: The srReconnectedChan branch currently calls
g.onNetworkChange() synchronously inside the guard select loop, which blocks
other cases; change it to dispatch the work asynchronously: when handling
srReconnectedChan, if g.onNetworkChange != nil spawn a goroutine to invoke it
instead of calling it inline and use a single-flight/coalescing mechanism (e.g.,
a singleflight.Group or an in-struct boolean guarded by a mutex/atomic) so
concurrent srReconnectedChan events only trigger one inflight onNetworkChange
invocation; ensure the async worker performs the existing reset iceBackoff and
recreate workerICE logic and that you respect ctx cancellation to avoid leaking
goroutines.

In `@client/internal/peer/worker_ice.go`:
- Around line 218-230: IsConnected reads lastKnownState under w.muxAgent while
onConnectionStateChange writes lastKnownState without locking, creating a race;
fix by protecting all accesses to lastKnownState with the same mutex
(w.muxAgent) — update onConnectionStateChange (the callback that sets
lastKnownState at/around where it currently assigns at lines ~545-563) to
acquire w.muxAgent.Lock() and defer w.muxAgent.Unlock() around writes (and any
reads), ensuring symmetry with IsConnected and preventing the close-or-skip
race.

In `@client/ui/client_ui.go`:
- Around line 656-666: The parseUint32Field function currently swallows
parse/overflow errors and returns 0 which can accidentally clear saved timeouts;
change parseUint32Field to return (uint32, error) instead of silently coercing —
validate strings.TrimSpace(text) then call strconv.ParseUint(..., 32) and return
an explicit error on failure (including overflow), and update all callers
(notably buildSetConfigRequest) to propagate or surface that error to the caller
(e.g., return/build an error response instead of using 0); apply the same change
to the other similar parser at the 741-750 block so all timeout parsing surfaces
errors instead of returning 0.

---

Nitpick comments:
In `@client/internal/lazyconn/manager/manager.go`:
- Line 68: Remove the dead inactivityThreshold field from the Manager struct and
eliminate its assignment in NewManager: delete the inactivityThreshold
declaration in the Manager type, remove any parameter/argument or line in
NewManager that sets m.inactivityThreshold, and ensure no other code reads it
(timeout logic should remain using resolvedTimeouts()). Update related
constructors/call sites to stop passing that value and run tests to confirm
nothing else references inactivityThreshold.

🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0a37e050-20cd-4834-a220-5d197ba3378d

📥 Commits

Reviewing files that changed from the base of the PR and between 6bf6777 and b2b91ef.

⛔ Files ignored due to path filters (1)

• client/proto/daemon.pb.go is excluded by !**/*.pb.go

📒 Files selected for processing (19)

• client/android/client.go
• client/android/preferences.go
• client/cmd/service.go
• client/cmd/service_installer.go
• client/internal/conn_mgr.go
• client/internal/conn_mgr_test.go
• client/internal/engine.go
• client/internal/lazyconn/manager/manager.go
• client/internal/peer/conn.go
• client/internal/peer/guard/guard.go
• client/internal/peer/handshaker.go
• client/internal/peer/ice_backoff.go
• client/internal/peer/ice_backoff_test.go
• client/internal/peer/worker_ice.go
• client/proto/daemon.proto
• client/server/server.go
• client/ui/client_ui.go
• client/ui/const.go
• client/ui/event_handler.go

💤 Files with no reviewable changes (2)

• client/ui/const.go
• client/ui/event_handler.go

🚧 Files skipped from review as they are similar to previous changes (1)

• client/internal/peer/handshaker.go

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Make connection-mode persistence failures abort the command.

These flags are not passed through buildServiceArguments(), so if applyConnectionModeFlagsToProfile fails here and execution continues, install/reconfigure completes with different connection-mode/timeout behavior than the user explicitly requested. This should return an error instead of downgrading to a warning.

Suggested change

-		if err := applyConnectionModeFlagsToProfile(cmd); err != nil {
-			cmd.PrintErrf("Warning: failed to persist connection-mode flags: %v\n", err)
-		}
+		if err := applyConnectionModeFlagsToProfile(cmd); err != nil {
+			return fmt.Errorf("persist connection-mode flags: %w", err)
+		}

Apply the same change in both the install and reconfigure paths.

Based on learnings methods returning errors should be propagated early rather than continuing with partially applied state.

Also applies to: 263-265

🤖 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 `@client/cmd/service_installer.go` around lines 135 - 139, The call to
applyConnectionModeFlagsToProfile currently logs a warning on failure, allowing
install/reconfigure to continue with inconsistent connection-mode state; change
this so that any error returned by applyConnectionModeFlagsToProfile is
propagated and causes the command to abort (return the error) instead of
printing a warning. Update both places where this call appears (the install path
around applyConnectionModeFlagsToProfile(cmd) and the reconfigure path) to
return the error up the call stack rather than using cmd.PrintErrf, ensuring the
command stops if persistence fails and no further install/reconfigure steps run.