From 68c481fa44a0790583f80ae8fa1d34e425b8d83b Mon Sep 17 00:00:00 2001 From: Bethuel Mmbaga Date: Sat, 14 Feb 2026 20:27:15 +0100 Subject: [PATCH] [management] Move service reload outside transaction in account settings update (#5325) Bug Fixes Network and DNS updates now defer service and reverse-proxy reloads until after account updates complete, preventing inconsistent proxy state and race conditions. Chores Removed automatic peer/broadcast updates immediately following bulk service reloads. Tests Added a test ensuring network-range changes complete without deadlock. --- .../modules/reverseproxy/manager/manager.go | 2 -- management/server/account.go | 10 ++++-- management/server/account_test.go | 33 +++++++++++++++++++ 3 files changed, 40 insertions(+), 5 deletions(-) diff --git a/management/internals/modules/reverseproxy/manager/manager.go b/management/internals/modules/reverseproxy/manager/manager.go index 2a93fdff6..535705a37 100644 --- a/management/internals/modules/reverseproxy/manager/manager.go +++ b/management/internals/modules/reverseproxy/manager/manager.go @@ -473,8 +473,6 @@ func (m *managerImpl) ReloadAllServicesForAccount(ctx context.Context, accountID m.proxyGRPCServer.SendServiceUpdateToCluster(service.ToProtoMapping(reverseproxy.Update, "", m.proxyGRPCServer.GetOIDCValidationConfig()), service.ProxyCluster) } - m.accountManager.UpdateAccountPeers(ctx, accountID) - return nil } diff --git a/management/server/account.go b/management/server/account.go index 7b858c223..1e35d4ad1 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -297,6 +297,7 @@ func (am *DefaultAccountManager) UpdateAccountSettings(ctx context.Context, acco var oldSettings *types.Settings var updateAccountPeers bool var groupChangesAffectPeers bool + var reloadReverseProxy bool err = am.Store.ExecuteInTransaction(ctx, func(transaction store.Store) error { var groupsUpdated bool @@ -327,9 +328,7 @@ func (am *DefaultAccountManager) UpdateAccountSettings(ctx context.Context, acco if err = am.reallocateAccountPeerIPs(ctx, transaction, accountID, newSettings.NetworkRange); err != nil { return err } - if err = am.reverseProxyManager.ReloadAllServicesForAccount(ctx, accountID); err != nil { - log.WithContext(ctx).Warnf("failed to reload all services for account %s: %v", accountID, err) - } + reloadReverseProxy = true updateAccountPeers = true } @@ -394,6 +393,11 @@ func (am *DefaultAccountManager) UpdateAccountSettings(ctx context.Context, acco } am.StoreEvent(ctx, userID, accountID, accountID, activity.AccountNetworkRangeUpdated, eventMeta) } + if reloadReverseProxy { + if err = am.reverseProxyManager.ReloadAllServicesForAccount(ctx, accountID); err != nil { + log.WithContext(ctx).Warnf("failed to reload all services for account %s: %v", accountID, err) + } + } if updateAccountPeers || extraSettingsChanged || groupChangesAffectPeers { go am.UpdateAccountPeers(ctx, accountID) diff --git a/management/server/account_test.go b/management/server/account_test.go index 44bb0fb1c..1cc0c9571 100644 --- a/management/server/account_test.go +++ b/management/server/account_test.go @@ -3918,3 +3918,36 @@ func TestAddNewUserToDomainAccountWithoutApproval(t *testing.T) { assert.False(t, user.PendingApproval, "User should not be pending approval") assert.Equal(t, existingAccountID, user.AccountID) } + +// TestDefaultAccountManager_UpdateAccountSettings_NetworkRangeChange verifies that +// changing NetworkRange via UpdateAccountSettings does not deadlock. +// The deadlock occurs because ReloadAllServicesForAccount is called inside a DB +// transaction but uses the main store connection, which blocks on the transaction lock. +func TestDefaultAccountManager_UpdateAccountSettings_NetworkRangeChange(t *testing.T) { + manager, _, err := createManager(t) + require.NoError(t, err) + + accountID, err := manager.GetAccountIDByUserID(context.Background(), auth.UserAuth{UserId: userID}) + require.NoError(t, err) + + ctx := context.Background() + + // Use a channel to detect if the call completes or hangs + done := make(chan error, 1) + go func() { + _, err := manager.UpdateAccountSettings(ctx, accountID, userID, &types.Settings{ + PeerLoginExpiration: time.Hour, + PeerLoginExpirationEnabled: true, + NetworkRange: netip.MustParsePrefix("10.100.0.0/16"), + Extra: &types.ExtraSettings{}, + }) + done <- err + }() + + select { + case err := <-done: + require.NoError(t, err, "UpdateAccountSettings should complete without error") + case <-time.After(10 * time.Second): + t.Fatal("UpdateAccountSettings deadlocked when changing NetworkRange") + } +}