Skip to content

[management] Move service reload outside transaction in account settings update #5325

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mlsmaycon merged 3 commits into from
Feb 14, 2026
+40 −5
Merged

[management] Move service reload outside transaction in account settings update #5325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7e4ebd5
move proxy service reload outside the transaction
bcmmbaga Feb 14, 2026
b13ef13
remove account peers update when reloading proxy services
bcmmbaga Feb 14, 2026
6d17ac8
add test
bcmmbaga Feb 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions management/internals/modules/reverseproxy/manager/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
10 changes: 7 additions & 3 deletions management/server/account.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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)
}
}
Comment on lines +396 to +400
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check how reverseProxyManager / SetServiceManager is initialized and where it's called
echo "=== SetServiceManager call sites ==="
rg -n 'SetServiceManager' --type go -C 3

echo ""
echo "=== reverseProxyManager usage ==="
rg -n 'reverseProxyManager' --type go -C 2

echo ""
echo "=== ReloadAllServicesForAccount interface definition ==="
ast-grep --pattern $'type Manager interface {
  $$$
  ReloadAllServicesForAccount($$$) $$$
  $$$
}'

Repository: netbirdio/netbird

Length of output: 29813

🏁 Script executed:

#!/bin/bash
# Check the initialization lifecycle and AfterInit pattern
echo "=== AfterInit callback pattern in modules.go ==="
sed -n '100,115p' management/internals/server/modules.go

echo ""
echo "=== BuildManager function (account manager creation) ==="
sed -n '95,130p' management/internals/server/modules.go

echo ""
echo "=== UpdateAccountSettings function signature ==="
rg -n 'func.*UpdateAccountSettings' --type go -A 2

echo ""
echo "=== How UpdateAccountSettings is typically called (checking for direct calls) ==="
rg -n 'UpdateAccountSettings' --type go | grep -v '_test.go' | head -20

Repository: netbirdio/netbird

Length of output: 5963

🏁 Script executed:

#!/bin/bash
# Check the BaseServer initialization lifecycle and AfterInit timing
echo "=== BaseServer structure and AfterInit definition ==="
rg -n 'type BaseServer' --type go -A 20 | head -40

echo ""
echo "=== AfterInit method implementation ==="
rg -n 'func.*AfterInit' --type go -B 2 -A 10

echo ""
echo "=== Server startup sequence - when handlers/HTTP listeners start ==="
rg -n 'func.*Start\|func.*Run' management/internals/server/base_server.go --type go -B 2 -A 5 | head -60

echo ""
echo "=== Check accounts_handler.go context and registration ==="
sed -n '1,50p' management/server/http/handlers/accounts/accounts_handler.go

Repository: netbirdio/netbird

Length of output: 4607

🏁 Script executed:

#!/bin/bash
# Check when afterInit callbacks are executed in Start()
echo "=== Full Start() method ==="
sed -n '92,180p' management/internals/server/server.go

echo ""
echo "=== HTTP routing/handler setup in Start() ==="
sed -n '92,250p' management/internals/server/server.go | tail -100

Repository: netbirdio/netbird

Length of output: 6922

🏁 Script executed:

#!/bin/bash
# Verify the initialization order guarantee more explicitly
echo "=== APIHandler creation in Start() ==="
rg -n 'APIHandler\(\)' management/internals/server/server.go -B 5 -A 5

echo ""
echo "=== When APIHandler is created and when HTTP listener starts accepting requests ==="
sed -n '150,170p' management/internals/server/server.go

Repository: netbirdio/netbird

Length of output: 1460

Add nil guard or document the initialization guarantee for reverseProxyManager.

The code at lines 396–400 calls am.reverseProxyManager.ReloadAllServicesForAccount() without a nil check. While the server initialization framework guarantees that SetServiceManager is called via AfterInit before the HTTP listener starts accepting requests (and thus before UpdateAccountSettings can be invoked), the lack of defensive nil checks creates fragility if the code is refactored, reused in different initialization contexts, or in test scenarios. Adding a nil guard (e.g., if am.reverseProxyManager != nil { ... }) would improve robustness, particularly given that other code paths in the codebase also use reverseProxyManager without similar guards.

🤖 Prompt for AI Agents 
In `@management/server/account.go` around lines 396 - 400, The call to
am.reverseProxyManager.ReloadAllServicesForAccount(...) in UpdateAccountSettings
is not nil-safe; add a defensive nil-check around am.reverseProxyManager (e.g.,
if am.reverseProxyManager != nil { ... }) before calling
ReloadAllServicesForAccount, or alternatively document/validate that
SetServiceManager must be called during initialization so reverseProxyManager is
guaranteed non-nil; prefer adding the nil guard to make UpdateAccountSettings
robust in tests and alternative init flows.


if updateAccountPeers || extraSettingsChanged || groupChangesAffectPeers {
go am.UpdateAccountPeers(ctx, accountID)
Expand Down
33 changes: 33 additions & 0 deletions management/server/account_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
}
Loading