mirror of
https://github.com/netbirdio/netbird.git
synced 2026-10-23 13:59:09 +02:00
[management] Skip inactivity expiration rescan on connect when a job is scheduled (#8287)
* [management] Skip inactivity expiration rescan when a job is scheduled Every SSO peer connect and disconnect cancelled the account's inactivity expiration job and re-armed it, loading every inactivity-enabled peer of the account each time. During a management deploy, when all peers reconnect, that is a full account scan per peer. A connecting peer cannot expire, and a disconnecting peer expires after every peer that was already disconnected, so neither can move the next run earlier. Connect and disconnect now leave an already scheduled job in place, as login expiration already does. Settings and per-peer toggle changes still cancel and reschedule, since they can move the run earlier. * [management] Keep inactivity rescan on disconnect A disconnect that lands while the inactivity job is finishing its last run saw the job as scheduled and skipped arming a new one; the scheduler then removed the job, leaving the disconnected peer without an inactivity timer. Only connect, where the peer cannot expire, now skips a scheduled job; disconnect cancels and re-arms as before. The test asserts on the synchronous Cancel call instead of sleeping.
This commit is contained in:
@@ -806,6 +806,21 @@ func (am *DefaultAccountManager) schedulePeerLoginExpiration(ctx context.Context
|
||||
}
|
||||
}
|
||||
|
||||
// schedulePeerInactivityExpiration arms the account's inactivity expiration job for a
|
||||
// connecting peer unless one is already scheduled: a connected peer cannot expire, so it
|
||||
// never moves the scheduled run earlier. Disconnects must not skip a running job, which may
|
||||
// be finishing its last run without seeing the newly disconnected peer.
|
||||
func (am *DefaultAccountManager) schedulePeerInactivityExpiration(ctx context.Context, accountID string) {
|
||||
if am.peerInactivityExpiry.IsSchedulerRunning(accountID) {
|
||||
log.WithContext(ctx).Tracef("peer inactivity expiration job for account %s is already scheduled", accountID)
|
||||
return
|
||||
}
|
||||
jobCtx := context.WithoutCancel(ctx)
|
||||
if nextRun, ok := am.getNextInactivePeerExpiration(ctx, accountID); ok {
|
||||
go am.peerInactivityExpiry.Schedule(jobCtx, nextRun, accountID, am.peerInactivityExpirationJob(jobCtx, accountID))
|
||||
}
|
||||
}
|
||||
|
||||
// peerInactivityExpirationJob marks login expired for all inactive peers and returns the minimum duration in which the next peer of the account will expire by inactivity if found
|
||||
func (am *DefaultAccountManager) peerInactivityExpirationJob(ctx context.Context, accountID string) func() (time.Duration, bool) {
|
||||
return func() (time.Duration, bool) {
|
||||
|
||||
@@ -13,6 +13,7 @@ import (
|
||||
"strconv"
|
||||
"strings"
|
||||
"sync"
|
||||
"sync/atomic"
|
||||
"testing"
|
||||
"testing/synctest"
|
||||
"time"
|
||||
@@ -2277,6 +2278,57 @@ func TestDefaultAccountManager_MarkPeerDisconnected_SchedulesInactivityExpiratio
|
||||
}
|
||||
}
|
||||
|
||||
func TestDefaultAccountManager_PeerConnect_KeepsScheduledInactivityExpiration(t *testing.T) {
|
||||
manager, _, err := createManager(t)
|
||||
require.NoError(t, err, "unable to create account manager")
|
||||
|
||||
accountID, err := manager.GetAccountIDByUserID(context.Background(), auth.UserAuth{UserId: userID})
|
||||
require.NoError(t, err, "unable to create an account")
|
||||
|
||||
key, err := wgtypes.GenerateKey()
|
||||
require.NoError(t, err, "unable to generate WireGuard key")
|
||||
peerPubKey := key.PublicKey().String()
|
||||
|
||||
_, _, _, _, err = manager.AddPeer(context.Background(), "", "", userID, &nbpeer.Peer{
|
||||
Key: peerPubKey,
|
||||
Meta: nbpeer.PeerSystemMeta{Hostname: "test-peer"},
|
||||
InactivityExpirationEnabled: true,
|
||||
}, false)
|
||||
require.NoError(t, err, "unable to add peer")
|
||||
|
||||
_, err = manager.UpdateAccountSettings(context.Background(), accountID, userID, &types.Settings{
|
||||
PeerLoginExpiration: time.Hour,
|
||||
PeerLoginExpirationEnabled: true,
|
||||
PeerInactivityExpiration: time.Hour,
|
||||
PeerInactivityExpirationEnabled: true,
|
||||
Extra: &types.ExtraSettings{},
|
||||
})
|
||||
require.NoError(t, err, "expecting to update account settings successfully but got error")
|
||||
|
||||
// The account already has an inactivity job scheduled. A connecting peer
|
||||
// cannot expire, so connect must leave that job alone instead of cancelling
|
||||
// it and re-scanning every peer of the account. Cancel runs synchronously,
|
||||
// so counting it observes the decision deterministically.
|
||||
var cancels atomic.Int32
|
||||
manager.peerInactivityExpiry = &MockScheduler{
|
||||
IsSchedulerRunningFunc: func(ID string) bool { return ID == accountID },
|
||||
CancelFunc: func(ctx context.Context, IDs []string) { cancels.Add(1) },
|
||||
ScheduleFunc: func(ctx context.Context, in time.Duration, ID string, job func() (nextRunIn time.Duration, reschedule bool)) {
|
||||
},
|
||||
}
|
||||
|
||||
streamStartTime := time.Now().UTC()
|
||||
err = manager.MarkPeerConnected(context.Background(), peerPubKey, accountID, streamStartTime.UnixNano(), nil)
|
||||
require.NoError(t, err, "unable to mark peer connected")
|
||||
assert.Zero(t, cancels.Load(), "connect must not cancel a scheduled inactivity job")
|
||||
|
||||
// A disconnect still cancels and re-arms: the running job may be finishing
|
||||
// its last run without having seen this peer as disconnected.
|
||||
err = manager.MarkPeerDisconnected(context.Background(), peerPubKey, accountID, streamStartTime.UnixNano())
|
||||
require.NoError(t, err, "unable to mark peer disconnected")
|
||||
assert.Equal(t, int32(1), cancels.Load(), "disconnect must cancel and re-arm the inactivity job")
|
||||
}
|
||||
|
||||
func TestDefaultAccountManager_MarkPeerDisconnected_SkipsInactivityExpirationWhenDisabled(t *testing.T) {
|
||||
manager, _, err := createManager(t)
|
||||
require.NoError(t, err, "unable to create account manager")
|
||||
|
||||
@@ -140,7 +140,7 @@ func (am *DefaultAccountManager) MarkPeerConnected(ctx context.Context, peerPubK
|
||||
return nil
|
||||
}
|
||||
|
||||
// schedulePeerExpirations reschedules the account's login/inactivity expiration
|
||||
// schedulePeerExpirations arms the account's login/inactivity expiration
|
||||
// timers for an SSO peer that just connected.
|
||||
func (am *DefaultAccountManager) schedulePeerExpirations(ctx context.Context, accountID string, peer *nbpeer.Peer) error {
|
||||
if !peer.AddedWithSSOLogin() {
|
||||
@@ -154,7 +154,7 @@ func (am *DefaultAccountManager) schedulePeerExpirations(ctx context.Context, ac
|
||||
am.schedulePeerLoginExpiration(ctx, accountID)
|
||||
}
|
||||
if peer.InactivityExpirationEnabled && settings.PeerInactivityExpirationEnabled {
|
||||
am.checkAndSchedulePeerInactivityExpiration(ctx, accountID)
|
||||
am.schedulePeerInactivityExpiration(ctx, accountID)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user