From f676dea097bd400d9e226e0b66a0503aa34ba0f4 Mon Sep 17 00:00:00 2001
From: rcourtman <courtmanr@gmail.com>
Date: Sat, 27 Jun 2026 20:22:04 +0100
Subject: [PATCH] fix(monitoring): prefer agent source for physical disk SMART
 metrics
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When a Proxmox node has an agent installed, SMART/temperature metrics are
written by the host agent under the agent's disk source ID. But
BuildMetricsTarget preferred the Proxmox source first, and Proxmox API
does not expose detailed SMART data. The mismatch meant the frontend
queried a metrics key that nothing writes, so disk metrics appeared empty.

Move the Agent source check ahead of Proxmox/TrueNAS for
ResourceTypePhysicalDisk, mirroring the existing pattern for
ResourceTypeAgent where agent source already wins over platform sources.

When a disk has a serial number, it is used as the canonical metric ID
regardless of source (PreferredPhysicalDiskMetricID), so the reorder
only affects disks without serials — exactly the case where source
priority determines the key.

Refs #1487
---
 internal/unifiedresources/metrics_targets.go  | 17 ++++--
 .../unifiedresources/metrics_targets_test.go  | 53 +++++++++++++++++++
 2 files changed, 65 insertions(+), 5 deletions(-)

diff --git a/internal/unifiedresources/metrics_targets.go b/internal/unifiedresources/metrics_targets.go
index 5865ccc48..96bc3341a 100644
--- a/internal/unifiedresources/metrics_targets.go
+++ b/internal/unifiedresources/metrics_targets.go
@@ -98,6 +98,18 @@ func BuildMetricsTarget(resource Resource, sourceTargets []SourceTarget) *Metric
 			return &MetricsTarget{ResourceType: "storage", ResourceID: st.SourceID}
 		}
 	case ResourceTypePhysicalDisk:
+		// The agent source must win over platform sources for physical
+		// disks: SMART/temperature metrics are written by the host agent
+		// under the agent's disk source ID (HostSMARTDiskSourceID in
+		// monitor_agents.go). Proxmox API does not expose detailed SMART
+		// data, so preferring its source ID here would point store readers
+		// at a key nothing writes and disk metrics would appear empty
+		// (GitHub issue #1487).
+		if st, ok := bySource[SourceAgent]; ok {
+			if resourceID := PhysicalDiskMetaMetricID(resource.PhysicalDisk, st.SourceID); resourceID != "" {
+				return &MetricsTarget{ResourceType: "disk", ResourceID: resourceID}
+			}
+		}
 		if st, ok := bySource[SourceProxmox]; ok {
 			if resourceID := PhysicalDiskMetaMetricID(resource.PhysicalDisk, st.SourceID); resourceID != "" {
 				return &MetricsTarget{ResourceType: "disk", ResourceID: resourceID}
@@ -108,11 +120,6 @@ func BuildMetricsTarget(resource Resource, sourceTargets []SourceTarget) *Metric
 				return &MetricsTarget{ResourceType: "disk", ResourceID: resourceID}
 			}
 		}
-		if st, ok := bySource[SourceAgent]; ok {
-			if resourceID := PhysicalDiskMetaMetricID(resource.PhysicalDisk, st.SourceID); resourceID != "" {
-				return &MetricsTarget{ResourceType: "disk", ResourceID: resourceID}
-			}
-		}
 	case ResourceTypePod:
 		if st, ok := bySource[SourceK8s]; ok {
 			if resourceID := CanonicalKubernetesPodMetricID(st.SourceID); resourceID != "" {
diff --git a/internal/unifiedresources/metrics_targets_test.go b/internal/unifiedresources/metrics_targets_test.go
index 16af9e1ad..f21e97fb2 100644
--- a/internal/unifiedresources/metrics_targets_test.go
+++ b/internal/unifiedresources/metrics_targets_test.go
@@ -372,6 +372,59 @@ func TestBuildMetricsTarget_UsesCanonicalPhysicalDiskMetricIDForTrueNAS(t *testi
 	}
 }
 
+func TestBuildMetricsTarget_AgentSourceWinsForPhysicalDiskMetrics(t *testing.T) {
+	// When a Proxmox node has an agent, the agent writes SMART/temperature
+	// metrics under its own disk source ID. The metrics target must resolve
+	// to the agent source ID, not the Proxmox source ID, or store readers
+	// query a key nothing writes and disk metrics appear empty (#1487).
+	// Without a serial, the source-specific fallback ID differs between
+	// sources, so source priority determines which key is returned.
+	target := BuildMetricsTarget(
+		Resource{
+			Type: ResourceTypePhysicalDisk,
+			PhysicalDisk: &PhysicalDiskMeta{
+				DiskType: "nvme",
+			},
+		},
+		[]SourceTarget{
+			{Source: SourceProxmox, SourceID: "pve-node:nvme0n1"},
+			{Source: SourceAgent, SourceID: "agent-uuid:nvme0n1"},
+		},
+	)
+	if target == nil {
+		t.Fatal("BuildMetricsTarget() returned nil")
+	}
+	if target.ResourceType != "disk" {
+		t.Fatalf("ResourceType = %q, want disk", target.ResourceType)
+	}
+	if target.ResourceID != "agent-uuid:nvme0n1" {
+		t.Fatalf("ResourceID = %q, want agent-uuid:nvme0n1 (agent source must win for physical disk SMART metrics)", target.ResourceID)
+	}
+}
+
+func TestBuildMetricsTarget_PhysicalDiskSerialWinsOverAllSources(t *testing.T) {
+	// When the disk has a serial number, it becomes the metric ID
+	// regardless of source, so the priority order doesn't matter.
+	target := BuildMetricsTarget(
+		Resource{
+			Type: ResourceTypePhysicalDisk,
+			PhysicalDisk: &PhysicalDiskMeta{
+				Serial: "SAMSUNG-SSD-870-500GB",
+			},
+		},
+		[]SourceTarget{
+			{Source: SourceProxmox, SourceID: "pve-node:sda"},
+			{Source: SourceAgent, SourceID: "agent-uuid:sda"},
+		},
+	)
+	if target == nil {
+		t.Fatal("BuildMetricsTarget() returned nil")
+	}
+	if target.ResourceID != "SAMSUNG-SSD-870-500GB" {
+		t.Fatalf("ResourceID = %q, want SAMSUNG-SSD-870-500GB (serial should be used as canonical ID)", target.ResourceID)
+	}
+}
+
 func TestBuildMetricsTarget_UsesCanonicalAppMetricIDForTrueNAS(t *testing.T) {
 	target := BuildMetricsTarget(
 		Resource{