NM-300: Clean up stale metrics on node deletion and disconnect

[abhishek9686]

Describe your changes

Provide Issue ticket number if applicable/not in title

Provide testing steps

Checklist before requesting a review

• My changes affect only 10 files or less.
• I have performed a self-review of my code and tested it.
• If it is a new feature, I have added thorough tests, my code is <= 1450 lines.
• If it is a bugfix, my code is <= 200 lines.
• My functions are <= 80 lines.
• I have had my code reviewed by a peer.
• My unit tests pass locally.
• Netmaker is awesome.

[tenki-reviewer]

Tenki Code Review - Complete

Files Reviewed: 7
Findings: 2

By Severity:

• 🟠 Medium: 2

---

The PR correctly propagates disconnected/deleted node metrics to peers and tightens PlatformUser network-permission checks, but contains a missing go keyword that causes the metrics monitor to block its own tick loop, and a newly-added goroutine that duplicates posture-check cleanup already performed by an existing helper.

Files Reviewed (7 files)

controllers/node.go
logic/metrics.go
logic/nodes.go
pro/controllers/users.go
pro/initialize.go
pro/logic/metrics.go
pro/logic/security.go

[tenki-reviewer]

Overview

This PR introduces two new peer-metrics helpers (SetPeerMetricsDisconnected, DeleteNodeMetricsFromPeers) wired through the existing function-variable dispatch pattern, updates node disconnect flows in controllers/node.go and logic/metrics.go, and adjusts the PlatformUser access-control path in pro/logic/security.go.

Finding 1 — Synchronous call blocks MetricsMonitor ticker (logic/metrics.go)

At logic/metrics.go:58, SetPeerMetricsDisconnected is called without the go keyword inside the metrics monitor's ticker goroutine. The function is O(N²): it fetches all nodes, then for each peer fetches and rewrites its metrics row. Calling it synchronously stalls the entire tick loop for every disconnected node found, potentially causing the monitor to fall far behind its configured interval. Both other call-sites (controllers/node.go:706 and controllers/node.go:894) correctly dispatch it with go.

Finding 2 — Duplicate posture-check cleanup on group update (pro/controllers/users.go)

The new goroutine at pro/controllers/users.go:650–658 iterates removed networks and calls proLogic.RemoveUserGroupFromPostureChecks for each. This is identical logic to what EnsureDefaultUserGroupNetworkPolicies already does at pro/logic/user_mgmt.go:1061–1065 — the same function that is already dispatched on line 647. The result is two concurrent calls to RemoveUserGroupFromPostureChecks per removed network on every group update.

Other Changes (no issues)

• connectivity.Latency = 999 sentinel on disconnect is consistent across all affected paths.
• pro/logic/security.go: splitting the PlatformUser GET-allow out of the combined Auditor || PlatformUser block and placing it after the netID check is logically correct — PlatformUser GET requests now correctly require a non-empty netID, matching the intent of the surrounding checks.
• pro/initialize.go: new function-variable assignments are correct and follow the existing pattern.

[tenki-reviewer]

🟠 SetPeerMetricsDisconnected called synchronously in MetricsMonitor loop (bug)

In logic/metrics.go line 58, SetPeerMetricsDisconnected(node.ID.String()) is called without the go keyword inside the metrics monitor's tick goroutine. The function iterates all nodes, fetches each peer's metrics row, and writes them back — an O(N²) database operation that will block the ticker from firing again until it finishes for every disconnected node in the current sweep. Both other call-sites (controllers/node.go:706 and controllers/node.go:894) correctly use go logic.SetPeerMetricsDisconnected(...). The omission here will cause the monitor to fall behind its configured interval in proportion to the number of disconnected nodes and total peer count.

💡 Suggestion: Add the go keyword to match all other call-sites and prevent blocking the ticker loop.

Suggested change

	SetPeerMetricsDisconnected(node.ID.String())
	go SetPeerMetricsDisconnected(node.ID.String())

📋 Prompt for AI Agents

In logic/metrics.go at line 58, change SetPeerMetricsDisconnected(node.ID.String()) to go SetPeerMetricsDisconnected(node.ID.String()). The function is an O(N²) DB operation (reads and writes every peer's metrics row); calling it synchronously blocks the MetricsMonitor ticker goroutine and causes it to fall behind its configured interval. All other call-sites (controllers/node.go lines 706 and 894) already use go for this call.

[tenki-reviewer]

🟠 RemoveUserGroupFromPostureChecks called twice per removed network on group update (bug)

The new goroutine added at pro/controllers/users.go:650–658 computes a removedNetworks vs keptNetworks diff and calls proLogic.RemoveUserGroupFromPostureChecks(userGroup.ID, netID) for each network that was removed. However, EnsureDefaultUserGroupNetworkPolicies — already dispatched on line 647 — performs the identical diff at pro/logic/user_mgmt.go:1061–1065 and calls RemoveUserGroupFromPostureChecks for those same networks. Every group update that removes a network now triggers two concurrent calls to RemoveUserGroupFromPostureChecks for each removed network. While the operation is currently idempotent (it only deletes), this is duplicated logic that will silently double-execute if the function ever gains side-effects.

💡 Suggestion: Remove the new goroutine entirely. EnsureDefaultUserGroupNetworkPolicies already handles posture-check cleanup for removed networks. If the intent was a standalone cleanup path, remove the equivalent code from EnsureDefaultUserGroupNetworkPolicies instead to avoid the duplication.

📋 Prompt for AI Agents

In pro/controllers/users.go, remove the goroutine at lines 650-658 (the go func() { ... }() block that calls proLogic.RemoveUserGroupFromPostureChecks). This logic is already executed inside proLogic.EnsureDefaultUserGroupNetworkPolicies (called on line 647): see pro/logic/user_mgmt.go lines 1061-1065, which perform the same removed-network diff and call RemoveUserGroupFromPostureChecks for each. Keeping both causes two concurrent removals per affected network on every group update.