From 48d91610566c60e2e1ee553589c473a1c0d48441 Mon Sep 17 00:00:00 2001 From: mlsmaycon Date: Sun, 9 Aug 2026 07:38:35 +0000 Subject: [PATCH] [management] Stamp proxy peer activity with the database clock The activity write took a Go-side timestamp, which is exactly what MarkPeerConnectedIfNewerSession documents as the cause of previous ordering bugs: a value read before the write can land after a connect that stamped CURRENT_TIMESTAMP, dragging LastSeen backwards. The write now uses the database clock like the other status writers, so the column only ever moves forward. The throttle is unaffected; it reads the peer already in hand and never needed the write's timestamp. --- management/internals/modules/peers/manager.go | 8 +++--- .../internals/modules/peers/manager_mock.go | 9 +++---- management/internals/shared/grpc/proxy.go | 2 +- .../shared/grpc/proxy_group_access_test.go | 10 +++----- management/server/store/sql_store.go | 12 ++++----- .../server/store/sql_store_activity_test.go | 25 +++++-------------- management/server/store/store.go | 5 ++-- management/server/store/store_mock.go | 8 +++--- 8 files changed, 32 insertions(+), 47 deletions(-) diff --git a/management/internals/modules/peers/manager.go b/management/internals/modules/peers/manager.go index fd234103e..fbe4a98a0 100644 --- a/management/internals/modules/peers/manager.go +++ b/management/internals/modules/peers/manager.go @@ -44,8 +44,8 @@ type Manager interface { // to. Used by the proxy's auth path to authorise a request by the calling // peer's group memberships. GetPeerWithGroups(ctx context.Context, accountID, peerID string) (*peer.Peer, []*types.Group, error) - // RefreshLastSeen records that a peer was seen at seenAt. - RefreshLastSeen(ctx context.Context, accountID, peerID string, seenAt time.Time) error + // RefreshLastSeen records that a peer was just seen. + RefreshLastSeen(ctx context.Context, accountID, peerID string) error } type managerImpl struct { @@ -130,8 +130,8 @@ func (m *managerImpl) GetPeerWithGroups(ctx context.Context, accountID, peerID s return p, groups, nil } -func (m *managerImpl) RefreshLastSeen(ctx context.Context, accountID, peerID string, seenAt time.Time) error { - return m.store.RefreshPeerLastSeen(ctx, accountID, peerID, seenAt) +func (m *managerImpl) RefreshLastSeen(ctx context.Context, accountID, peerID string) error { + return m.store.RefreshPeerLastSeen(ctx, accountID, peerID) } func (m *managerImpl) DeletePeers(ctx context.Context, accountID string, peerIDs []string, userID string, checkConnected bool) error { diff --git a/management/internals/modules/peers/manager_mock.go b/management/internals/modules/peers/manager_mock.go index 21f91c06a..65055b15b 100644 --- a/management/internals/modules/peers/manager_mock.go +++ b/management/internals/modules/peers/manager_mock.go @@ -8,7 +8,6 @@ import ( context "context" net "net" reflect "reflect" - time "time" gomock "github.com/golang/mock/gomock" network_map "github.com/netbirdio/netbird/management/internals/controllers/network_map" @@ -176,17 +175,17 @@ func (mr *MockManagerMockRecorder) GetPeersByGroupIDs(ctx, accountID, groupsIDs } // RefreshLastSeen mocks base method. -func (m *MockManager) RefreshLastSeen(ctx context.Context, accountID, peerID string, seenAt time.Time) error { +func (m *MockManager) RefreshLastSeen(ctx context.Context, accountID, peerID string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "RefreshLastSeen", ctx, accountID, peerID, seenAt) + ret := m.ctrl.Call(m, "RefreshLastSeen", ctx, accountID, peerID) ret0, _ := ret[0].(error) return ret0 } // RefreshLastSeen indicates an expected call of RefreshLastSeen. -func (mr *MockManagerMockRecorder) RefreshLastSeen(ctx, accountID, peerID, seenAt interface{}) *gomock.Call { +func (mr *MockManagerMockRecorder) RefreshLastSeen(ctx, accountID, peerID interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RefreshLastSeen", reflect.TypeOf((*MockManager)(nil).RefreshLastSeen), ctx, accountID, peerID, seenAt) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RefreshLastSeen", reflect.TypeOf((*MockManager)(nil).RefreshLastSeen), ctx, accountID, peerID) } // SetAccountManager mocks base method. diff --git a/management/internals/shared/grpc/proxy.go b/management/internals/shared/grpc/proxy.go index 28c326677..58303de96 100644 --- a/management/internals/shared/grpc/proxy.go +++ b/management/internals/shared/grpc/proxy.go @@ -2094,7 +2094,7 @@ func (s *ProxyServiceServer) recordPeerSeen(ctx context.Context, accountID strin return } - if err := s.peersManager.RefreshLastSeen(ctx, accountID, peer.ID, time.Now().UTC()); err != nil { + if err := s.peersManager.RefreshLastSeen(ctx, accountID, peer.ID); err != nil { log.WithContext(ctx).Debugf("record proxy activity for peer %s: %v", peer.ID, err) } } diff --git a/management/internals/shared/grpc/proxy_group_access_test.go b/management/internals/shared/grpc/proxy_group_access_test.go index fc0fbc6ac..50d147ff0 100644 --- a/management/internals/shared/grpc/proxy_group_access_test.go +++ b/management/internals/shared/grpc/proxy_group_access_test.go @@ -171,15 +171,15 @@ type mockTunnelPeersManager struct { seenMarks []seenMark } -// seenMark records a RefreshLastSeen call. +// seenMark records a RefreshLastSeen call. The timestamp is the database's, so +// there is nothing from the caller to assert beyond who was marked. type seenMark struct { accountID string peerID string - at time.Time } -func (m *mockTunnelPeersManager) RefreshLastSeen(_ context.Context, accountID, peerID string, seenAt time.Time) error { - m.seenMarks = append(m.seenMarks, seenMark{accountID: accountID, peerID: peerID, at: seenAt}) +func (m *mockTunnelPeersManager) RefreshLastSeen(_ context.Context, accountID, peerID string) error { + m.seenMarks = append(m.seenMarks, seenMark{accountID: accountID, peerID: peerID}) return nil } @@ -848,8 +848,6 @@ func TestValidateTunnelPeerRecordsActivity(t *testing.T) { mark := peersManager.seenMarks[0] assert.Equal(t, accountID, mark.accountID, "activity must be recorded against the service account") assert.Equal(t, peerID, mark.peerID, "activity must be recorded against the calling peer") - assert.Equal(t, time.UTC, mark.at.Location(), "timestamps are written in UTC") - assert.WithinDuration(t, time.Now().UTC(), mark.at, time.Minute, "seen timestamp should be now") }) } } diff --git a/management/server/store/sql_store.go b/management/server/store/sql_store.go index 3578a3ff2..b0620d337 100644 --- a/management/server/store/sql_store.go +++ b/management/server/store/sql_store.go @@ -604,15 +604,15 @@ func (s *SqlStore) ApproveAccountPeers(ctx context.Context, accountID string) (i // peer_status_session_started_at belong to the sync stream that owns the // session, and a blind write here would corrupt the fencing // MarkPeerConnectedIfNewerSession relies on. -func (s *SqlStore) RefreshPeerLastSeen(ctx context.Context, accountID, peerID string, seenAt time.Time) error { - if seenAt.IsZero() { - return nil - } - +// +// LastSeen comes from the database clock for the same reason it does there: a +// Go-side timestamp is taken before the write and can land after a connect that +// used CURRENT_TIMESTAMP, dragging the column backwards. +func (s *SqlStore) RefreshPeerLastSeen(ctx context.Context, accountID, peerID string) error { result := s.db.WithContext(ctx). Model(&nbpeer.Peer{}). Where(accountAndIDQueryCondition, accountID, peerID). - Update("peer_status_last_seen", seenAt) + Update("peer_status_last_seen", gorm.Expr("CURRENT_TIMESTAMP")) if result.Error != nil { return status.Errorf(status.Internal, "refresh peer last seen: %v", result.Error) } diff --git a/management/server/store/sql_store_activity_test.go b/management/server/store/sql_store_activity_test.go index 596139394..e01d41218 100644 --- a/management/server/store/sql_store_activity_test.go +++ b/management/server/store/sql_store_activity_test.go @@ -104,27 +104,15 @@ func TestRefreshUserLastLoginUnknownUserIsNotAnError(t *testing.T) { func TestRefreshPeerLastSeen(t *testing.T) { ctx := context.Background() store := newActivityTestStore(t) - require.NoError(t, store.AddPeerToAccount(ctx, activityPeer(time.Date(2026, 3, 1, 9, 0, 0, 0, time.UTC)))) - - seenAt := time.Date(2026, 3, 1, 12, 0, 0, 0, time.UTC) - require.NoError(t, store.RefreshPeerLastSeen(ctx, activityAccountID, "activityPeer", seenAt)) - - peer, err := store.GetPeerByID(ctx, LockingStrengthNone, activityAccountID, "activityPeer") - require.NoError(t, err) - assert.WithinDuration(t, seenAt, peer.Status.LastSeen.UTC(), time.Second, "unexpected stored last seen") -} - -func TestRefreshPeerLastSeenZeroIsIgnored(t *testing.T) { - ctx := context.Background() - store := newActivityTestStore(t) - stored := time.Date(2026, 3, 1, 9, 0, 0, 0, time.UTC) + stored := time.Now().UTC().Add(-3 * time.Hour) require.NoError(t, store.AddPeerToAccount(ctx, activityPeer(stored))) - require.NoError(t, store.RefreshPeerLastSeen(ctx, activityAccountID, "activityPeer", time.Time{})) + require.NoError(t, store.RefreshPeerLastSeen(ctx, activityAccountID, "activityPeer")) peer, err := store.GetPeerByID(ctx, LockingStrengthNone, activityAccountID, "activityPeer") require.NoError(t, err) - assert.WithinDuration(t, stored, peer.Status.LastSeen.UTC(), time.Second, "a zero timestamp must not clear last seen") + assert.WithinDuration(t, time.Now().UTC(), peer.Status.LastSeen.UTC(), time.Minute, "last seen should be stamped at write time") + assert.True(t, peer.Status.LastSeen.After(stored), "last seen must move forward") } // TestRefreshPeerLastSeenLeavesSessionStateAlone pins the column boundary: the @@ -139,12 +127,11 @@ func TestRefreshPeerLastSeenLeavesSessionStateAlone(t *testing.T) { stored.Status.SessionStartedAt = 1234567890 require.NoError(t, store.AddPeerToAccount(ctx, stored)) - seenAt := time.Date(2026, 3, 1, 12, 0, 0, 0, time.UTC) - require.NoError(t, store.RefreshPeerLastSeen(ctx, activityAccountID, "activityPeer", seenAt)) + require.NoError(t, store.RefreshPeerLastSeen(ctx, activityAccountID, "activityPeer")) peer, err := store.GetPeerByID(ctx, LockingStrengthNone, activityAccountID, "activityPeer") require.NoError(t, err) - assert.WithinDuration(t, seenAt, peer.Status.LastSeen.UTC(), time.Second, "last seen should move forward") + assert.WithinDuration(t, time.Now().UTC(), peer.Status.LastSeen.UTC(), time.Minute, "last seen should move forward") assert.True(t, peer.Status.Connected, "connected flag must survive an activity write") assert.Equal(t, int64(1234567890), peer.Status.SessionStartedAt, "session token must survive an activity write") } diff --git a/management/server/store/store.go b/management/server/store/store.go index ebd6bcc3c..ecf6c3c00 100644 --- a/management/server/store/store.go +++ b/management/server/store/store.go @@ -185,11 +185,12 @@ type Store interface { // Returns true when the update happened, false when this stream lost // the race against a newer session. MarkPeerConnectedIfNewerSession(ctx context.Context, accountID, peerID string, newSessionStartedAt int64) (bool, error) - // RefreshPeerLastSeen records that a peer was seen at seenAt. Connected and + // RefreshPeerLastSeen records that a peer was just seen, stamping the + // database clock like the other status writers. Connected and // SessionStartedAt are left alone, so this never interferes with the // session-ownership protocol MarkPeerConnectedIfNewerSession implements. // Callers decide how often to call it; the store does not throttle. - RefreshPeerLastSeen(ctx context.Context, accountID, peerID string, seenAt time.Time) error + RefreshPeerLastSeen(ctx context.Context, accountID, peerID string) error // MarkPeerDisconnectedIfSameSession sets the peer to disconnected and // resets SessionStartedAt to zero, but only when the stored // SessionStartedAt equals the given sessionStartedAt. LastSeen is diff --git a/management/server/store/store_mock.go b/management/server/store/store_mock.go index 2fe261547..852cd0ec8 100644 --- a/management/server/store/store_mock.go +++ b/management/server/store/store_mock.go @@ -3204,17 +3204,17 @@ func (mr *MockStoreMockRecorder) MarkProxyAccessTokenUsed(ctx, tokenID interface } // RefreshPeerLastSeen mocks base method. -func (m *MockStore) RefreshPeerLastSeen(ctx context.Context, accountID, peerID string, seenAt time.Time) error { +func (m *MockStore) RefreshPeerLastSeen(ctx context.Context, accountID, peerID string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "RefreshPeerLastSeen", ctx, accountID, peerID, seenAt) + ret := m.ctrl.Call(m, "RefreshPeerLastSeen", ctx, accountID, peerID) ret0, _ := ret[0].(error) return ret0 } // RefreshPeerLastSeen indicates an expected call of RefreshPeerLastSeen. -func (mr *MockStoreMockRecorder) RefreshPeerLastSeen(ctx, accountID, peerID, seenAt interface{}) *gomock.Call { +func (mr *MockStoreMockRecorder) RefreshPeerLastSeen(ctx, accountID, peerID interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RefreshPeerLastSeen", reflect.TypeOf((*MockStore)(nil).RefreshPeerLastSeen), ctx, accountID, peerID, seenAt) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RefreshPeerLastSeen", reflect.TypeOf((*MockStore)(nil).RefreshPeerLastSeen), ctx, accountID, peerID) } // RefreshUserLastLogin mocks base method.