bring back last_seen in database #2579

kradalby · 2025-05-09T12:31:54Z

ghost · 2025-05-09T12:31:57Z

Pull Request Revisions

Revision	Description
r6	Updated node online status handling Refactored node online status to use pointer, modified error messages, and adjusted database migration logic
r5	Added Online field using ptr.To(false) Updated test files to include `Online` field with `ptr.To(false)` across multiple test cases in mapper package
r4	Removed unnecessary time.Sleep call Removed a hardcoded one-minute sleep in the TestAuthKeyLogoutAndReloginSameUser test function, keeping the comment about reconnection conditions
r3	No changes since last revision
r2	Added LastSeen column to Node Implemented database migration and methods to track and persist the LastSeen timestamp for nodes, including database schema update and new setter methods
r1	Updated node online status handling Modified node type from pointer boolean to direct boolean and adjusted LastSeen logic across multiple files

✅ AI review completed for r6

Help

React with emojis to give feedback on AI-generated reviews:

👍 means the feedback was helpful and actionable
👎 means the feedback was incorrect or unhelpful

💬 Replying to feedback with a comment helps us improve the system. Your input also contributes to shaping future interactions with the AI reviewer.

We'd love to hear from you—reach out anytime at [email protected].

ghost · 2025-05-09T13:19:09Z

hscontrol/db/db.go

+				Migrate: func(tx *gorm.DB) error {
+					_ = tx.Migrator().AddColumn(&types.Node{}, "last_seen")
+
+					return nil
+				},


In the migration function in hscontrol/db/db.go, you're ignoring the error from tx.Migrator().AddColumn(). This could potentially hide migration failures. Consider either properly handling the error or at least adding a comment explaining why it's safe to ignore.

ghost · 2025-05-09T13:19:10Z

hscontrol/db/node.go

+// SetLastSeen sets a node's last seen field indicating that we
+// have recently communicating with this node.
+func (hsdb *HSDatabase) SetLastSeen(nodeID types.NodeID, lastSeen time.Time) error {
+	return hsdb.Write(func(tx *gorm.DB) error {
+		return SetLastSeen(tx, nodeID, lastSeen)
+	})
+}
+
+// SetLastSeen sets a node's last seen field indicating that we
+// have recently communicating with this node.
+func SetLastSeen(tx *gorm.DB, nodeID types.NodeID, lastSeen time.Time) error {
+	return tx.Model(&types.Node{}).Where("id = ?", nodeID).Update("last_seen", lastSeen).Error
+}


In hscontrol/db/node.go, you have duplicated documentation comments for the SetLastSeen function. Consider removing the redundancy to maintain cleaner code.

ghost · 2025-05-09T13:19:10Z

hscontrol/grpcv1.go

 			resp.Online = true
+			resp.LastSeen = timestamppb.New(time.Now())


In hscontrol/grpcv1.go, you're setting LastSeen to the current time for online nodes, which differs from the pattern in hscontrol/mapper/tail.go where a similar operation is done. Consider extracting this logic into a shared utility function to ensure consistent behavior across the codebase.

ghost · 2025-05-09T13:19:10Z

hscontrol/poll.go

+	if node.LastSeen != nil {
+		h.db.SetLastSeen(node.ID, *node.LastSeen)
+	}


In hscontrol/poll.go, you're updating the LastSeen field in the database only if it's not nil. However, there's no clear validation ensuring that LastSeen gets initialized properly. Consider adding more robust handling for the case when LastSeen is unexpectedly nil.

ghost · 2025-05-09T13:19:10Z

hscontrol/mapper/mapper.go

 	for _, peer := range peers {
 		online := m.notif.IsLikelyConnected(peer.ID)
-		peer.IsOnline = &online
+		peer.IsOnline = online


In hscontrol/mapper/mapper.go, you're changing from a pointer bool to a direct bool for IsOnline, which is a good simplification, but this could potentially cause issues if any other parts of the code expect IsOnline to be nullable. Have you verified compatibility across all usage sites?

ghost · 2025-05-09T13:19:10Z

hscontrol/mapper/tail.go

+	if node.IsOnline {
+		// If the node is online, return the current time.
+		tNode.LastSeen = ptr.To(time.Now())
+	} else {
 		// LastSeen is only set when node is
 		// not connected to the control server.
 		tNode.LastSeen = node.LastSeen


In hscontrol/mapper/tail.go, the conditional logic for setting LastSeen has been updated but there could be a potential issue with consistency. While online nodes get the current time, offline nodes use the stored LastSeen which might be nil. Consider handling the nil case explicitly to avoid potential nil pointer dereferences later.

ghost · 2025-05-09T13:19:10Z

integration/auth_key_test.go

 			// if the server is not running with HTTPS, we have to wait a bit before
 			// reconnection as the newest Tailscale client has a measure that will only
 			// reconnect over HTTPS if they saw a noise connection previously.


In integration/auth_key_test.go, removing the time.Sleep(time.Minute) might introduce flakiness in tests if the test relies on certain timing behaviors. Have you verified that the tests remain stable without this delay?

ghost · 2025-05-09T13:21:50Z

hscontrol/types/node.go


 	// LastSeen is when the node was last in contact with
 	// headscale. It is best effort and not persisted.
-	LastSeen *time.Time `gorm:"-"`
-
-	// DEPRECATED: Use the ApprovedRoutes field instead.
-	// TODO(kradalby): remove when ApprovedRoutes is used all over the code.
-	// Routes []Route `gorm:"constraint:OnDelete:CASCADE;"`
+	LastSeen *time.Time `gorm:"column:last_seen"`


In hscontrol/types/node.go, the LastSeen field is now persisted to the database which is good, but consider adding validation in relevant functions to handle potential database errors when LastSeen is NULL in the database (for newly created nodes or after migrations).

ghost · 2025-05-09T13:21:50Z

hscontrol/mapper/tail.go

+	} else {
 		// LastSeen is only set when node is
 		// not connected to the control server.
 		tNode.LastSeen = node.LastSeen


In hscontrol/mapper/tail.go, consider adding a guard clause or default value when handling LastSeen for offline nodes: tNode.LastSeen = node.LastSeen. If node.LastSeen is nil, this could lead to unexpected behavior in downstream code that expects a valid time value.

ghost · 2025-05-09T13:21:50Z

hscontrol/db/db.go

+			// Add back last_seen column to node table.
+			{
+				ID: "202505091439",
+				Migrate: func(tx *gorm.DB) error {
+					_ = tx.Migrator().AddColumn(&types.Node{}, "last_seen")
+
+					return nil
+				},
+				Rollback: func(db *gorm.DB) error { return nil },
+			},


In hscontrol/db/db.go, you're adding a new migration but not validating whether the column already exists before attempting to add it. Consider adding a condition to check if the column exists to avoid potential errors when running the migration multiple times.

Fixes juanfont#2574 Signed-off-by: Kristoffer Dalby <[email protected]>

Signed-off-by: Kristoffer Dalby <[email protected]>

ghost · 2025-05-10T06:28:29Z

integration/auth_key_test.go

+				assertLastSeenSet(t, node)
+			}

 			allAddrs := lo.Map(allIps, func(x netip.Addr, index int) string {


In integration/auth_key_test.go, allIps is first fetched on line 48-49 but then used much later in the test on line 127. Between these points, clients log out and log back in, potentially changing the IP addresses. Consider either refreshing the IP list before use or using a more reliable identifier for nodes.

kradalby force-pushed the kradalby/last-seen-online branch from c2570ad to 629e71a Compare May 9, 2025 13:08

kradalby marked this pull request as ready for review May 9, 2025 13:09

kradalby requested a review from juanfont as a code owner May 9, 2025 13:09

kradalby force-pushed the kradalby/last-seen-online branch from 629e71a to 11a7f5d Compare May 9, 2025 13:18

ghost reviewed May 9, 2025

View reviewed changes

kradalby force-pushed the kradalby/last-seen-online branch from 11a7f5d to 3ff0d58 Compare May 9, 2025 13:21

ghost reviewed May 9, 2025

View reviewed changes

kradalby changed the title ~~lastseen and online tweaks~~ bring back last_seen in database May 9, 2025

nblock approved these changes May 9, 2025

View reviewed changes

kradalby added 2 commits May 10, 2025 08:11

db: add back last_seen to the database

109e412

Fixes juanfont#2574 Signed-off-by: Kristoffer Dalby <[email protected]>

integration: ensure last_seen is set

4414dfc

Signed-off-by: Kristoffer Dalby <[email protected]>

kradalby force-pushed the kradalby/last-seen-online branch from 3ff0d58 to 4414dfc Compare May 10, 2025 06:27

ghost reviewed May 10, 2025

View reviewed changes

kradalby merged commit 43943ae into juanfont:main May 10, 2025
143 of 146 checks passed

		resp.Online = true
		resp.LastSeen = timestamppb.New(time.Now())

Uh oh!

bring back last_seen in database #2579

bring back last_seen in database #2579

Uh oh!

Conversation

kradalby commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented May 9, 2025 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Revisions

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kradalby commented May 9, 2025 •

edited

Loading

ghost commented May 9, 2025 •

edited by ghost

Loading