[PR #6409] [CLOSED] [client] Fixes DisableAutoConnect semantics #29498

Closed
opened 2026-08-05 08:08:10 -04:00 by saavagebueno · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/netbirdio/netbird/pull/6409
Author: @riccardomanfrin
Created: 6/11/2026
Status: Closed

Base: mainHead: disable_auto_connect_no_client_stop


📝 Commits (2)

  • c2c23f6 Fixes DisableAutoConnect semantics
  • ab16673 Merge branch 'main' into disable_auto_connect_no_client_stop

📊 Changes

1 file changed (+4 additions, -11 deletions)

View changed files

📝 client/server/server.go (+4 -11)

📄 Description

Describe your changes

Clarify the semantics of the DisableAutoConnect flag in the client
daemon. The flag previously short-circuited every code path that
considered spawning the connection goroutine — service Start, the Up
RPC handler, and future coming MDM-driven engine restart.

The flag's intent is narrower: it governs whether the daemon should
automatically connect at service start. An explicit user request,
or a policy refresh applied to an already-running engine, should not be
subordinated to the flag.

New semantics:

  1. Service Start + DisableAutoConnect=true → daemon stays
    idle, no connection goroutine spawned, clientRunning stays
    false. The state RPC reports StatusIdle.
  2. Service Start + DisableAutoConnect=false → daemon spawns
    connectWithRetryRuns, retry loop runs until ctx cancelled.
  3. Up RPC (any value of DisableAutoConnect) → flag ignored.
    The user explicitly asked to connect — by definition not "auto".
  4. MDM-triggered restart (any value of DisableAutoConnect)
    flag ignored. An MDM policy change applies new config to an
    already-running engine; treated as an implicit Up.
  5. Down / Stop / Logout → cancels ctx; cleanupConnection
    clears clientRunning. DisableAutoConnect is not involved.

Code changes:

  • client/server/server.go: removed the DisableAutoConnect branch
    inside connectWithRetryRuns so the retry loop no longer
    collapses to a single-shot when the flag is set. The flag is now
    only consulted at the top of Start() (the early-return path that
    sets StatusIdle).

No public issue. Semantic divergence between the flag name and its
behaviour surfaced during the MDM integration review
(https://github.com/netbirdio/netbird/pull/6374) — DisableAutoConnect
was being read as a runtime kill switch in addition to a startup
gate. This PR confines it to its documented purpose so the MDM PR
does not have to choose between two reasonable but incompatible
interpretations.

Stack

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)

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

Documentation

Select exactly one:

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

The DisableAutoConnect flag is already documented as a
startup-only auto-connect gate (CLI help text and ConfigInput
field comment). The previous implementation diverged from the
documented intent; this PR aligns the implementation with the
existing docs, so no doc update is required. The behavioural
contract becomes: setting the flag prevents the boot-time
connection only — explicit Up requests and MDM-driven restarts
remain unaffected.

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

N/A

Summary by CodeRabbit

  • Bug Fixes
    • Updated connection retry behavior so connection attempts consistently follow the backoff-based retry flow.
    • Added improved debug logging when a client run exits with an error, making connection termination issues easier to diagnose.

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

## 📋 Pull Request Information **Original PR:** https://github.com/netbirdio/netbird/pull/6409 **Author:** [@riccardomanfrin](https://github.com/riccardomanfrin) **Created:** 6/11/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `disable_auto_connect_no_client_stop` --- ### 📝 Commits (2) - [`c2c23f6`](https://github.com/netbirdio/netbird/commit/c2c23f66fed26d9a6a3c95488c19f7cdbb47f8fd) Fixes DisableAutoConnect semantics - [`ab16673`](https://github.com/netbirdio/netbird/commit/ab166735458635a2df559ab5adb7f6e262343da2) Merge branch 'main' into disable_auto_connect_no_client_stop ### 📊 Changes **1 file changed** (+4 additions, -11 deletions) <details> <summary>View changed files</summary> 📝 `client/server/server.go` (+4 -11) </details> ### 📄 Description ## Describe your changes Clarify the semantics of the `DisableAutoConnect` flag in the client daemon. The flag previously short-circuited every code path that considered spawning the connection goroutine — service Start, the Up RPC handler, and future coming MDM-driven engine restart. The flag's intent is narrower: it governs whether the daemon should automatically connect **at service start**. An explicit user request, or a policy refresh applied to an already-running engine, should not be subordinated to the flag. New semantics: 1. **Service Start + `DisableAutoConnect=true`** → daemon stays idle, no connection goroutine spawned, `clientRunning` stays `false`. The state RPC reports `StatusIdle`. 2. **Service Start + `DisableAutoConnect=false`** → daemon spawns `connectWithRetryRuns`, retry loop runs until ctx cancelled. 3. **Up RPC (any value of `DisableAutoConnect`)** → flag ignored. The user explicitly asked to connect — by definition not "auto". 4. **MDM-triggered restart (any value of `DisableAutoConnect`)** → flag ignored. An MDM policy change applies new config to an already-running engine; treated as an implicit Up. 5. **Down / Stop / Logout** → cancels ctx; `cleanupConnection` clears `clientRunning`. `DisableAutoConnect` is not involved. Code changes: - `client/server/server.go`: removed the `DisableAutoConnect` branch inside `connectWithRetryRuns` so the retry loop no longer collapses to a single-shot when the flag is set. The flag is now only consulted at the top of `Start()` (the early-return path that sets `StatusIdle`). ## Issue ticket number and link No public issue. Semantic divergence between the flag name and its behaviour surfaced during the MDM integration review (https://github.com/netbirdio/netbird/pull/6374) — `DisableAutoConnect` was being read as a runtime kill switch in addition to a startup gate. This PR confines it to its documented purpose so the MDM PR does not have to choose between two reasonable but incompatible interpretations. ## Stack <!-- branch-stack --> ### Checklist - [ ] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [x] It is a refactor - [ ] Created tests that fail without the change (if possible) > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) The `DisableAutoConnect` flag is already documented as a startup-only auto-connect gate (CLI help text and ConfigInput field comment). The previous implementation diverged from the documented intent; this PR aligns the implementation with the existing docs, so no doc update is required. The behavioural contract becomes: setting the flag prevents the boot-time connection only — explicit Up requests and MDM-driven restarts remain unaffected. ### Docs PR URL (required if "docs added" is checked) N/A <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Updated connection retry behavior so connection attempts consistently follow the backoff-based retry flow. * Added improved debug logging when a client run exits with an error, making connection termination issues easier to diagnose. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
saavagebueno added the pull-request label 2026-08-05 08:08:10 -04:00
Sign in to join this conversation.
No Label pull-request
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DYNR/netbird#29498