Merge branch 'feature/check-cert-locker-before-acme' into test/proxy-fixes

client/server/server.go

@@ -1625,14 +1625,9 @@ func (s *Server) GetFeatures(ctx context.Context, msg *proto.GetFeaturesRequest)
 
 func (s *Server) connect(ctx context.Context, config *profilemanager.Config, statusRecorder *peer.Status, doInitialAutoUpdate bool, runningChan chan struct{}) error {
 	log.Tracef("running client connection")
-	client := internal.NewConnectClient(ctx, config, statusRecorder, doInitialAutoUpdate)
-	client.SetSyncResponsePersistence(s.persistSyncResponse)
-
-	s.mutex.Lock()
-	s.connectClient = client
-	s.mutex.Unlock()
-
-	if err := client.Run(runningChan, s.logFile); err != nil {
+	s.connectClient = internal.NewConnectClient(ctx, config, statusRecorder, doInitialAutoUpdate)
+	s.connectClient.SetSyncResponsePersistence(s.persistSyncResponse)
+	if err := s.connectClient.Run(runningChan, s.logFile); err != nil {
 		return err
 	}
 	return nil

client/server/server_connect_test.go

@@ -1,187 +0,0 @@
-package server
-
-import (
-	"context"
-	"sync"
-	"testing"
-	"time"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-
-	"github.com/netbirdio/netbird/client/internal"
-	"github.com/netbirdio/netbird/client/internal/peer"
-	"github.com/netbirdio/netbird/client/proto"
-)
-
-func newTestServer() *Server {
-	return &Server{
-		rootCtx:        context.Background(),
-		statusRecorder: peer.NewRecorder(""),
-	}
-}
-
-func newDummyConnectClient(ctx context.Context) *internal.ConnectClient {
-	return internal.NewConnectClient(ctx, nil, nil, false)
-}
-
-// TestConnectSetsClientWithMutex validates that connect() sets s.connectClient
-// under mutex protection so concurrent readers see a consistent value.
-func TestConnectSetsClientWithMutex(t *testing.T) {
-	s := newTestServer()
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
-
-	// Manually simulate what connect() does (without calling Run which panics without full setup)
-	client := newDummyConnectClient(ctx)
-
-	s.mutex.Lock()
-	s.connectClient = client
-	s.mutex.Unlock()
-
-	// Verify the assignment is visible under mutex
-	s.mutex.Lock()
-	assert.Equal(t, client, s.connectClient, "connectClient should be set")
-	s.mutex.Unlock()
-}
-
-// TestConcurrentConnectClientAccess validates that concurrent reads of
-// s.connectClient under mutex don't race with a write.
-func TestConcurrentConnectClientAccess(t *testing.T) {
-	s := newTestServer()
-	ctx := context.Background()
-	client := newDummyConnectClient(ctx)
-
-	var wg sync.WaitGroup
-	nilCount := 0
-	setCount := 0
-	var mu sync.Mutex
-
-	// Start readers
-	for i := 0; i < 50; i++ {
-		wg.Add(1)
-		go func() {
-			defer wg.Done()
-			s.mutex.Lock()
-			c := s.connectClient
-			s.mutex.Unlock()
-
-			mu.Lock()
-			defer mu.Unlock()
-			if c == nil {
-				nilCount++
-			} else {
-				setCount++
-			}
-		}()
-	}
-
-	// Simulate connect() writing under mutex
-	time.Sleep(5 * time.Millisecond)
-	s.mutex.Lock()
-	s.connectClient = client
-	s.mutex.Unlock()
-
-	wg.Wait()
-
-	assert.Equal(t, 50, nilCount+setCount, "all goroutines should complete without panic")
-}
-
-// TestCleanupConnection_ClearsConnectClient validates that cleanupConnection
-// properly nils out connectClient.
-func TestCleanupConnection_ClearsConnectClient(t *testing.T) {
-	s := newTestServer()
-	_, cancel := context.WithCancel(context.Background())
-	s.actCancel = cancel
-
-	s.connectClient = newDummyConnectClient(context.Background())
-	s.clientRunning = true
-
-	err := s.cleanupConnection()
-	require.NoError(t, err)
-
-	assert.Nil(t, s.connectClient, "connectClient should be nil after cleanup")
-}
-
-// TestCleanState_NilConnectClient validates that CleanState doesn't panic
-// when connectClient is nil.
-func TestCleanState_NilConnectClient(t *testing.T) {
-	s := newTestServer()
-	s.connectClient = nil
-	s.profileManager = nil // will cause error if it tries to proceed past the nil check
-
-	// Should not panic — the nil check should prevent calling Status() on nil
-	assert.NotPanics(t, func() {
-		_, _ = s.CleanState(context.Background(), &proto.CleanStateRequest{All: true})
-	})
-}
-
-// TestDeleteState_NilConnectClient validates that DeleteState doesn't panic
-// when connectClient is nil.
-func TestDeleteState_NilConnectClient(t *testing.T) {
-	s := newTestServer()
-	s.connectClient = nil
-	s.profileManager = nil
-
-	assert.NotPanics(t, func() {
-		_, _ = s.DeleteState(context.Background(), &proto.DeleteStateRequest{All: true})
-	})
-}
-
-// TestDownThenUp_StaleRunningChan documents the known state issue where
-// clientRunningChan from a previous connection is already closed, causing
-// waitForUp() to return immediately on reconnect.
-func TestDownThenUp_StaleRunningChan(t *testing.T) {
-	s := newTestServer()
-
-	// Simulate state after a successful connection
-	s.clientRunning = true
-	s.clientRunningChan = make(chan struct{})
-	close(s.clientRunningChan) // closed when engine started
-	s.clientGiveUpChan = make(chan struct{})
-	s.connectClient = newDummyConnectClient(context.Background())
-
-	_, cancel := context.WithCancel(context.Background())
-	s.actCancel = cancel
-
-	// Simulate Down(): cleanupConnection sets connectClient = nil
-	s.mutex.Lock()
-	err := s.cleanupConnection()
-	s.mutex.Unlock()
-	require.NoError(t, err)
-
-	// After cleanup: connectClient is nil, clientRunning still true
-	// (goroutine hasn't exited yet)
-	s.mutex.Lock()
-	assert.Nil(t, s.connectClient, "connectClient should be nil after cleanup")
-	assert.True(t, s.clientRunning, "clientRunning still true until goroutine exits")
-	s.mutex.Unlock()
-
-	// waitForUp() returns immediately due to stale closed clientRunningChan
-	ctx, ctxCancel := context.WithTimeout(context.Background(), 2*time.Second)
-	defer ctxCancel()
-
-	waitDone := make(chan error, 1)
-	go func() {
-		_, err := s.waitForUp(ctx)
-		waitDone <- err
-	}()
-
-	select {
-	case err := <-waitDone:
-		assert.NoError(t, err, "waitForUp returns success on stale channel")
-		// But connectClient is still nil — this is the stale state issue
-		s.mutex.Lock()
-		assert.Nil(t, s.connectClient, "connectClient is nil despite waitForUp success")
-		s.mutex.Unlock()
-	case <-time.After(1 * time.Second):
-		t.Fatal("waitForUp should have returned immediately due to stale closed channel")
-	}
-}
-
-// TestConnectClient_EngineNilOnFreshClient validates that a newly created
-// ConnectClient has nil Engine (before Run is called).
-func TestConnectClient_EngineNilOnFreshClient(t *testing.T) {
-	client := newDummyConnectClient(context.Background())
-	assert.Nil(t, client.Engine(), "engine should be nil on fresh ConnectClient")
-}

client/server/state.go

@@ -39,7 +39,7 @@ func (s *Server) ListStates(_ context.Context, _ *proto.ListStatesRequest) (*pro
 
 // CleanState handles cleaning of states (performing cleanup operations)
 func (s *Server) CleanState(ctx context.Context, req *proto.CleanStateRequest) (*proto.CleanStateResponse, error) {
-	if s.connectClient != nil && (s.connectClient.Status() == internal.StatusConnected || s.connectClient.Status() == internal.StatusConnecting) {
+	if s.connectClient.Status() == internal.StatusConnected || s.connectClient.Status() == internal.StatusConnecting {
 		return nil, status.Errorf(codes.FailedPrecondition, "cannot clean state while connecting or connected, run 'netbird down' first.")
 	}
 
@@ -82,7 +82,7 @@ func (s *Server) CleanState(ctx context.Context, req *proto.CleanStateRequest) (
 
 // DeleteState handles deletion of states without cleanup
 func (s *Server) DeleteState(ctx context.Context, req *proto.DeleteStateRequest) (*proto.DeleteStateResponse, error) {
-	if s.connectClient != nil && (s.connectClient.Status() == internal.StatusConnected || s.connectClient.Status() == internal.StatusConnecting) {
+	if s.connectClient.Status() == internal.StatusConnected || s.connectClient.Status() == internal.StatusConnecting {
 		return nil, status.Errorf(codes.FailedPrecondition, "cannot clean state while connecting or connected, run 'netbird down' first.")
 	}
 

client/ui/client_ui.go

@@ -323,7 +323,7 @@ type serviceClient struct {
 
 	exitNodeMu           sync.Mutex
 	mExitNodeItems       []menuHandler
-	exitNodeRetryCancel  context.CancelFunc
+	exitNodeStates       []exitNodeState
 	mExitNodeDeselectAll *systray.MenuItem
 	logFile              string
 	wLoginURL            fyne.Window
@@ -924,7 +924,7 @@ func (s *serviceClient) updateStatus() error {
 			s.mDown.Enable()
 			s.mNetworks.Enable()
 			s.mExitNode.Enable()
-			s.startExitNodeRefresh()
+			go s.updateExitNodes()
 			systrayIconState = true
 		case status.Status == string(internal.StatusConnecting):
 			s.setConnectingStatus()
@@ -985,7 +985,6 @@ func (s *serviceClient) setDisconnectedStatus() {
 	s.mUp.Enable()
 	s.mNetworks.Disable()
 	s.mExitNode.Disable()
-	s.cancelExitNodeRetry()
 	go s.updateExitNodes()
 }
 

client/ui/event_handler.go

@@ -100,7 +100,8 @@ func (h *eventHandler) handleConnectClick() {
 
 func (h *eventHandler) handleDisconnectClick() {
 	h.client.mDown.Disable()
-	h.client.cancelExitNodeRetry()
+
+	h.client.exitNodeStates = []exitNodeState{}
 
 	if h.client.connectCancel != nil {
 		log.Debugf("cancelling ongoing connect operation")

client/ui/network.go

@@ -6,6 +6,7 @@ import (
 	"context"
 	"fmt"
 	"runtime"
+	"slices"
 	"sort"
 	"strings"
 	"time"
@@ -33,6 +34,11 @@ const (
 
 type filter string
 
+type exitNodeState struct {
+	id       string
+	selected bool
+}
+
 func (s *serviceClient) showNetworksUI() {
 	s.wNetworks = s.app.NewWindow("Networks")
 	s.wNetworks.SetOnClosed(s.cancel)
@@ -329,75 +335,16 @@ func (s *serviceClient) updateNetworksBasedOnDisplayTab(tabs *container.AppTabs,
 	s.updateNetworks(grid, f)
 }
 
-// startExitNodeRefresh initiates exit node menu refresh after connecting.
-// On Windows, TrayOpenedCh is not supported by the systray library, so we use
-// a background poller to keep exit nodes in sync while connected.
-// On macOS/Linux, TrayOpenedCh handles refreshes on each tray open.
-func (s *serviceClient) startExitNodeRefresh() {
-	s.cancelExitNodeRetry()
-
-	if runtime.GOOS == "windows" {
-		ctx, cancel := context.WithCancel(s.ctx)
-		s.exitNodeMu.Lock()
-		s.exitNodeRetryCancel = cancel
-		s.exitNodeMu.Unlock()
-
-		go s.pollExitNodes(ctx)
-	} else {
-		go s.updateExitNodes()
-	}
-}
-
-func (s *serviceClient) cancelExitNodeRetry() {
-	s.exitNodeMu.Lock()
-	if s.exitNodeRetryCancel != nil {
-		s.exitNodeRetryCancel()
-		s.exitNodeRetryCancel = nil
-	}
-	s.exitNodeMu.Unlock()
-}
-
-// pollExitNodes periodically refreshes exit nodes while connected.
-// Uses a short initial interval to catch routes from the management sync,
-// then switches to a longer interval for ongoing updates.
-func (s *serviceClient) pollExitNodes(ctx context.Context) {
-	// Initial fast polling to catch routes as they appear after connect.
-	for i := 0; i < 5; i++ {
-		if s.updateExitNodes() {
-			break
-		}
-		select {
-		case <-ctx.Done():
-			return
-		case <-time.After(2 * time.Second):
-		}
-	}
-
-	ticker := time.NewTicker(10 * time.Second)
-	defer ticker.Stop()
-
-	for {
-		select {
-		case <-ctx.Done():
-			return
-		case <-ticker.C:
-			s.updateExitNodes()
-		}
-	}
-}
-
-// updateExitNodes fetches exit nodes from the daemon and recreates the menu.
-// Returns true if exit nodes were found.
-func (s *serviceClient) updateExitNodes() bool {
+func (s *serviceClient) updateExitNodes() {
 	conn, err := s.getSrvClient(defaultFailTimeout)
 	if err != nil {
 		log.Errorf("get client: %v", err)
-		return false
+		return
 	}
 	exitNodes, err := s.getExitNodes(conn)
 	if err != nil {
 		log.Errorf("get exit nodes: %v", err)
-		return false
+		return
 	}
 
 	s.exitNodeMu.Lock()
@@ -407,14 +354,28 @@ func (s *serviceClient) updateExitNodes() bool {
 
 	if len(s.mExitNodeItems) > 0 {
 		s.mExitNode.Enable()
-		return true
+	} else {
+		s.mExitNode.Disable()
 	}
-
-	s.mExitNode.Disable()
-	return false
 }
 
 func (s *serviceClient) recreateExitNodeMenu(exitNodes []*proto.Network) {
+	var exitNodeIDs []exitNodeState
+	for _, node := range exitNodes {
+		exitNodeIDs = append(exitNodeIDs, exitNodeState{
+			id:       node.ID,
+			selected: node.Selected,
+		})
+	}
+
+	sort.Slice(exitNodeIDs, func(i, j int) bool {
+		return exitNodeIDs[i].id < exitNodeIDs[j].id
+	})
+	if slices.Equal(s.exitNodeStates, exitNodeIDs) {
+		log.Debug("Exit node menu already up to date")
+		return
+	}
+
 	for _, node := range s.mExitNodeItems {
 		node.cancel()
 		node.Hide()
@@ -452,6 +413,8 @@ func (s *serviceClient) recreateExitNodeMenu(exitNodes []*proto.Network) {
 		go s.handleChecked(ctx, node.ID, menuItem)
 	}
 
+	s.exitNodeStates = exitNodeIDs
+
 	if showDeselectAll {
 		s.mExitNode.AddSeparator()
 		deselectAllItem := s.mExitNode.AddSubMenuItem("Deselect All", "Deselect All")

management/internals/shared/grpc/server.go

@@ -330,12 +330,13 @@ func (s *Server) Sync(req *proto.EncryptedMessage, srv proto.ManagementService_S
 
 	s.secretsManager.SetupRefresh(ctx, accountID, peer.ID)
 
-	unlock()
-	unlock = nil
-
 	if s.appMetrics != nil {
 		s.appMetrics.GRPCMetrics().CountSyncRequestDuration(time.Since(reqStart), accountID)
 	}
+
+	unlock()
+	unlock = nil
+
 	log.WithContext(ctx).Debugf("Sync took %s", time.Since(reqStart))
 
 	s.syncSem.Add(-1)
@@ -742,6 +743,13 @@ func (s *Server) Login(ctx context.Context, req *proto.EncryptedMessage) (*proto
 
 	log.WithContext(ctx).Debugf("Login request from peer [%s] [%s]", req.WgPubKey, sRealIP)
 
+	defer func() {
+		if s.appMetrics != nil {
+			s.appMetrics.GRPCMetrics().CountLoginRequestDuration(time.Since(reqStart), accountID)
+		}
+		log.WithContext(ctx).Debugf("Login took %s", time.Since(reqStart))
+	}()
+
 	if loginReq.GetMeta() == nil {
 		msg := status.Errorf(codes.FailedPrecondition,
 			"peer system meta has to be provided to log in. Peer %s, remote addr %s", peerKey.String(), realIP)
@@ -791,11 +799,6 @@ func (s *Server) Login(ctx context.Context, req *proto.EncryptedMessage) (*proto
 		return nil, status.Errorf(codes.Internal, "failed logging in peer")
 	}
 
-	if s.appMetrics != nil {
-		s.appMetrics.GRPCMetrics().CountLoginRequestDuration(time.Since(reqStart), accountID)
-	}
-	log.WithContext(ctx).Debugf("Login took %s", time.Since(reqStart))
-
 	return &proto.EncryptedMessage{
 		WgPubKey: key.PublicKey().String(),
 		Body:     encryptedResp,

management/server/activity/codes.go

@@ -221,11 +221,11 @@ const (
 	AccountPeerExposeDisabled Activity = 115
 
 	// DomainAdded indicates that a user added a custom domain
-	DomainAdded Activity = 118
+	DomainAdded Activity = 116
 	// DomainDeleted indicates that a user deleted a custom domain
-	DomainDeleted Activity = 119
+	DomainDeleted Activity = 117
 	// DomainValidated indicates that a custom domain was validated
-	DomainValidated Activity = 120
+	DomainValidated Activity = 118
 
 	AccountDeleted Activity = 99999
 )

proxy/internal/acme/manager.go

@@ -135,24 +135,45 @@ func (mgr *Manager) AddDomain(d domain.Domain, accountID, serviceID string) {
 
 // prefetchCertificate proactively triggers certificate generation for a domain.
 // It acquires a distributed lock to prevent multiple replicas from issuing
-// duplicate ACME requests. The second replica will block until the first
-// finishes, then find the certificate in the cache.
+// duplicate ACME requests. If the certificate appears in the cache while waiting
+// for the lock, it cancels the wait and uses the cached certificate.
 func (mgr *Manager) prefetchCertificate(d domain.Domain) {
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
 	defer cancel()
 
 	name := d.PunycodeString()
 
+	if mgr.certExistsInCache(ctx, name) {
+		mgr.logger.Infof("certificate for domain %q already exists in cache before lock attempt", name)
+		mgr.loadAndFinalizeCachedCert(ctx, d, name)
+		return
+	}
+
+	go mgr.pollCacheAndCancel(ctx, name, cancel)
+
+	// Acquire lock
 	mgr.logger.Infof("acquiring cert lock for domain %q", name)
 	lockStart := time.Now()
 	unlock, err := mgr.locker.Lock(ctx, name)
 	if err != nil {
-		mgr.logger.Warnf("acquire cert lock for domain %q, proceeding without lock: %v", name, err)
+		if mgr.certExistsInCache(context.Background(), name) {
+			mgr.logger.Infof("certificate for domain %q appeared in cache while waiting for lock", name)
+			mgr.loadAndFinalizeCachedCert(context.Background(), d, name)
+			return
+		}
+		mgr.logger.Warnf("acquire cert lock for domain %q: %v", name, err)
+		// Continue without lock
 	} else {
 		mgr.logger.Infof("acquired cert lock for domain %q in %s", name, time.Since(lockStart))
 		defer unlock()
 	}
 
+	if mgr.certExistsInCache(ctx, name) {
+		mgr.logger.Infof("certificate for domain %q already exists in cache after lock acquisition, skipping ACME request", name)
+		mgr.loadAndFinalizeCachedCert(ctx, d, name)
+		return
+	}
+
 	hello := &tls.ClientHelloInfo{
 		ServerName: name,
 		Conn:       &dummyConn{ctx: ctx},
@@ -209,6 +230,78 @@ func (mgr *Manager) setDomainState(d domain.Domain, state domainState, errMsg st
 	}
 }
 
+// certExistsInCache checks if a certificate exists on the shared disk for the given domain.
+func (mgr *Manager) certExistsInCache(ctx context.Context, domain string) bool {
+	if mgr.Cache == nil {
+		return false
+	}
+	_, err := mgr.Cache.Get(ctx, domain)
+	return err == nil
+}
+
+// pollCacheAndCancel periodically checks if a certificate appears in the cache.
+// If found, it cancels the context to abort the lock wait.
+func (mgr *Manager) pollCacheAndCancel(ctx context.Context, domain string, cancel context.CancelFunc) {
+	ticker := time.NewTicker(5 * time.Second)
+	defer ticker.Stop()
+
+	for {
+		select {
+		case <-ctx.Done():
+			return
+		case <-ticker.C:
+			if mgr.certExistsInCache(context.Background(), domain) {
+				mgr.logger.Debugf("cert detected in cache for domain %q, cancelling lock wait", domain)
+				cancel()
+				return
+			}
+		}
+	}
+}
+
+// loadAndFinalizeCachedCert loads a certificate from cache and updates domain state.
+func (mgr *Manager) loadAndFinalizeCachedCert(ctx context.Context, d domain.Domain, name string) {
+	hello := &tls.ClientHelloInfo{
+		ServerName: name,
+		Conn:       &dummyConn{ctx: ctx},
+	}
+
+	cert, err := mgr.GetCertificate(hello)
+	if err != nil {
+		mgr.logger.Warnf("load cached certificate for domain %q: %v", name, err)
+		mgr.setDomainState(d, domainFailed, err.Error())
+		return
+	}
+
+	mgr.setDomainState(d, domainReady, "")
+
+	now := time.Now()
+	if cert != nil && cert.Leaf != nil {
+		leaf := cert.Leaf
+		mgr.logger.Infof("loaded cached certificate for domain %q: serial=%s SANs=%v notBefore=%s, notAfter=%s, now=%s",
+			name,
+			leaf.SerialNumber.Text(16),
+			leaf.DNSNames,
+			leaf.NotBefore.UTC().Format(time.RFC3339),
+			leaf.NotAfter.UTC().Format(time.RFC3339),
+			now.UTC().Format(time.RFC3339),
+		)
+		mgr.logCertificateDetails(name, leaf, now)
+	} else {
+		mgr.logger.Infof("loaded cached certificate for domain %q", name)
+	}
+
+	mgr.mu.RLock()
+	info := mgr.domains[d]
+	mgr.mu.RUnlock()
+
+	if info != nil && mgr.certNotifier != nil {
+		if err := mgr.certNotifier.NotifyCertificateIssued(ctx, info.accountID, info.serviceID, name); err != nil {
+			mgr.logger.Warnf("notify certificate ready for domain %q: %v", name, err)
+		}
+	}
+}
+
 // logCertificateDetails logs certificate validity and SCT timestamps.
 func (mgr *Manager) logCertificateDetails(domain string, cert *x509.Certificate, now time.Time) {
 	if cert.NotBefore.After(now) {