From b506c520232db93113af102dcc2ff2a6e4b3ab1e Mon Sep 17 00:00:00 2001 From: mlsmaycon Date: Sun, 9 Aug 2026 11:06:48 +0000 Subject: [PATCH] [management] Record activity for a peer that was never seen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit peer_status_last_seen is nullable — Status is an embedded pointer, so a peer stored without one leaves the column NULL — and NULL loses the cutoff comparison, so such a peer was silently skipped forever instead of recording its first activity. --- management/server/store/sql_store.go | 6 ++++-- .../server/store/sql_store_activity_test.go | 20 +++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/management/server/store/sql_store.go b/management/server/store/sql_store.go index 198377312..de358984d 100644 --- a/management/server/store/sql_store.go +++ b/management/server/store/sql_store.go @@ -611,12 +611,14 @@ func (s *SqlStore) ApproveAccountPeers(ctx context.Context, accountID string) (i // // staleBefore carries the caller's throttle into the same statement, so // concurrent requests for one peer collapse into a single write instead of -// each racing on its own stale read. +// each racing on its own stale read. The column is nullable — Status is an +// embedded pointer, so a peer stored without one leaves it NULL — and NULL +// loses every comparison, hence the explicit branch for a peer never seen. func (s *SqlStore) RefreshPeerLastSeen(ctx context.Context, accountID, peerID string, staleBefore time.Time) (bool, error) { result := s.db.WithContext(ctx). Model(&nbpeer.Peer{}). Where(accountAndIDQueryCondition, accountID, peerID). - Where("peer_status_last_seen < ?", staleBefore). + Where("(peer_status_last_seen IS NULL OR peer_status_last_seen < ?)", staleBefore). Update("peer_status_last_seen", gorm.Expr("CURRENT_TIMESTAMP")) if result.Error != nil { return false, 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 77a211bf4..b7aef47b1 100644 --- a/management/server/store/sql_store_activity_test.go +++ b/management/server/store/sql_store_activity_test.go @@ -65,6 +65,26 @@ func TestRefreshPeerLastSeenHonoursCutoff(t *testing.T) { assert.WithinDuration(t, stored, peer.Status.LastSeen.UTC(), time.Second, "last seen must be left where it was") } +// TestRefreshPeerLastSeenRecordsNeverSeenPeer covers the nullable column. Status +// is an embedded pointer, so a peer stored without one leaves last seen NULL, +// and NULL loses the cutoff comparison — such a peer would never record its +// first activity. +func TestRefreshPeerLastSeenRecordsNeverSeenPeer(t *testing.T) { + ctx := context.Background() + store := newActivityTestStore(t) + stored := activityPeer(time.Time{}) + stored.Status = nil + require.NoError(t, store.AddPeerToAccount(ctx, stored)) + + refreshed, err := store.RefreshPeerLastSeen(ctx, activityAccountID, "activityPeer", time.Now().UTC().Add(-time.Hour)) + require.NoError(t, err) + assert.True(t, refreshed, "a peer that was never seen must record its first activity") + + peer, err := store.GetPeerByID(ctx, LockingStrengthNone, activityAccountID, "activityPeer") + require.NoError(t, err) + assert.WithinDuration(t, time.Now().UTC(), peer.Status.LastSeen.UTC(), time.Minute, "last seen should be stamped at write time") +} + // TestRefreshPeerLastSeenLeavesSessionStateAlone pins the column boundary: the // connected flag and the session token belong to the sync stream that owns the // peer's session, and a blind write here would corrupt its fencing. This is why