Keep a merged disk's temperature under its own row's state

The registry merges a physical disk's rows (Proxmox, host agent SMART,
Unraid inventory), choosing the temperature by source preference and
merging field states separately. When two rows carry different
temperatures the shown value could sit under the other row's
"available" state: a silent agent's retained reading carried over a
Proxmox row that collected its own reading now, in either ingest order.
Risk, history and the UI then treated a frozen reading as current.

pairPhysicalDiskTemperatureState runs at the end of each merge step:
when the merged state says collected but the row the shown value came
from says it was not, the merged state becomes that row's own. It never
changes a value and never grants availability, so it cannot revive a
withdrawn reading or override a resumed agent's fresh one; rows with
the same value keep the merged state.

This is exact for the two rows of a step. The merged disk keeps one
state per field, so with three or more rows a withdrawal an earlier
step replaced is invisible to a later one; closing that needs each
reading's own state tracked across merges. Preferring another source's
current value instead would need an ordering between rows that the
merge does not record.

With this in place the Unraid-native row no longer needs to withhold an
"available" state to avoid masking a SMART row, so it now carries the
same derived state as a SMART row borrowing its reading.

docs/release-control/v6/internal/subsystems/unified-resources.md

@@ -3576,20 +3576,30 @@ state the Proxmox row still carries from the same source
 past its reporting lease must not stay "collected" through the Proxmox row's
 copy of the agent's own state, including when its host is down and no disk
 poll refreshes that copy.
+Each merge step also stops the shown temperature borrowing another row's
+availability (`pairPhysicalDiskTemperatureState`). The value is still chosen
+by source preference and the states merged as above, so when the two rows of a
+step carry different temperatures the shown value could sit under the other
+row's `available` state, for example a silent agent's retained reading carried
+over a Proxmox row that collected its own reading now. When the merged state
+says collected but the row the shown value came from says it was not, the
+merged state becomes that row's own. The rule never changes a value and never
+grants availability; rows with the same value keep the merged state. It sees
+only the two rows of one step: the merged disk keeps one state per field, so
+with three or more rows for one disk a withdrawal an earlier step replaced is
+not visible to a later step, and the earlier step's merged state stands in for
+its row. Tracking each reading's own state across merges would close that.
 Unraid array-inventory rows carry no per-field provenance, so the adapter
-derives the state of a temperature taken from one: `unavailable` from `unraid`
+derives the state of a positive temperature taken from one (a row without a
+temperature claims no state): `unavailable` from `unraid`
 with "host agent stopped reporting" once `State.ExpireHostTelemetry` has marked
 the host offline, or with "disk is reported spun down" for a spun-down disk,
 and otherwise `available`. The value is kept as last-known context; it is
 collected again only when a reporting host sends a positive reading for a disk
-that is no longer spun down. A SMART row that falls back to
-the inventory reading because it has none of its own takes that state with it.
-The Unraid-native disk row records only the withdrawing states, never
-`available`: the registry chooses a merged disk's temperature and its state
-separately, and an `available` claim from the provenance-less row would
-outrank the SMART row's explicit state for the same disk when the SMART
-reading is the one shown. A withdrawn inventory temperature also stops
-counting as heat in the disk's risk assessment.
+that is no longer spun down. The Unraid-native disk row carries that state, and
+so does a SMART row that falls back to the inventory reading because it has
+none of its own. The adapter's risk assessment of an Unraid row no longer counts
+a withdrawn inventory temperature as heat.
 
 Cross-source correlation compares normalized serial and WWN values across
 fields without truncation, allowing a PVE bare-hex array-volume serial to join

internal/monitoring/physical_disk_roundtrip_test.go

@@ -687,11 +687,10 @@ func TestSilentUnraidAgentDiskTemperatureIsRetainedButNotCollected(t *testing.T)
 		return got
 	}
 
-	// A reporting agent's readings are collected. The inventory-only disk
-	// keeps no state of its own (it counts as collected); the SMART rows that
-	// borrow the inventory reading take its provenance.
+	// A reporting agent's inventory readings are collected, including where
+	// a SMART row borrows them.
 	collected := map[string]diskinventory.FieldStatus{
-		"UNRAID-ONLY1":  {},
+		"UNRAID-ONLY1":  diskinventory.Available("unraid"),
 		"UNRAID-SMART2": diskinventory.Available("unraid"),
 		"UNRAID-SAS3":   diskinventory.Available("unraid"),
 	}

internal/unifiedresources/adapter_coverage_test.go

@@ -1049,10 +1049,9 @@ func TestMetricPercent(t *testing.T) {
 
 // Unraid inventory temperatures have no agent provenance, so the adapter
 // derives their state from the host's reporting lease and the disk's spin
-// state. A native row records only a state that withdraws its reading, a SMART
-// row that borrows the inventory reading takes the inventory's state, and a
-// SMART row with its own reading keeps the agent's state. A withdrawn reading
-// never counts as heat in the disk's risk.
+// state. The native row and a SMART row that borrows the inventory reading
+// both carry it, and a SMART row with its own reading keeps the agent's state.
+// A withdrawn reading never counts as heat in the disk's risk.
 func TestUnraidDiskTemperatureFollowsTheHostLease(t *testing.T) {
 	smartWithout := models.HostDiskSMART{Device: "/dev/sdc", Serial: "UNRAID-1", Collection: &diskinventory.CollectionStatus{
 		Serial:      diskinventory.Available("smartctl"),
@@ -1081,7 +1080,7 @@ func TestUnraidDiskTemperatureFollowsTheHostLease(t *testing.T) {
 		native   diskinventory.FieldStatus
 		borrowed diskinventory.FieldStatus
 	}{
-		{"reporting", "online", false, diskinventory.FieldStatus{}, diskinventory.Available("unraid")},
+		{"reporting", "online", false, diskinventory.Available("unraid"), diskinventory.Available("unraid")},
 		{"silent agent", "offline", false, stopped, stopped},
 		{"spun down", "online", true, spunDown, spunDown},
 	} {
@@ -1131,10 +1130,8 @@ func TestUnraidDiskTemperatureFollowsTheHostLease(t *testing.T) {
 }
 
 // The registry merges an Unraid inventory row and the agent's SMART row for the
-// same disk, choosing the temperature and the collection state separately. A
-// reporting host's inventory row must not lend the merged disk an "available"
-// state when the reading it shows is the SMART row's, and that row says it was
-// not collected.
+// same disk. When they disagree, the inventory's "available" never lends
+// itself to the SMART row's uncollected reading.
 func TestUnraidInventoryRowDoesNotMaskTheSMARTRowState(t *testing.T) {
 	state := models.NewState()
 	state.UpsertHost(models.Host{
@@ -1158,7 +1155,8 @@ func TestUnraidInventoryRowDoesNotMaskTheSMARTRowState(t *testing.T) {
 	if len(disks) != 1 {
 		t.Fatalf("physical disks = %d, want the inventory and SMART rows merged into one", len(disks))
 	}
-	if got := disks[0]; got.Temperature != 40 || diskinventory.TemperatureCollected(got.Temperature, got.Collection) {
-		t.Fatalf("merged disk presents the SMART row's uncollected reading as collected: temperature=%d collection=%+v", got.Temperature, got.Collection)
+	if got := disks[0]; got.Temperature != 40 || got.Collection == nil ||
+		got.Collection.Temperature != diskinventory.Unavailable("smartctl", "disk is in standby") {
+		t.Fatalf("merged disk presents the SMART row's uncollected reading under another state: temperature=%d collection=%+v", got.Temperature, got.Collection)
 	}
 }

internal/unifiedresources/adapters.go

@@ -806,12 +806,8 @@ func resourceFromHostUnraidPhysicalDisk(host models.Host, disk models.HostUnraid
 	health := unraidPhysicalDiskHealth(disk)
 	assessment := assessUnraidPhysicalDisk(host, disk)
 	sizeBytes := unraidDiskSizeBytes(host, disk)
-	// The row records only a state that withdraws its reading. A positive
-	// claim from this provenance-less row would outrank another source's
-	// explicit state for the same disk when the registry merges them, even
-	// where that source's temperature is the one shown.
 	var collection *diskinventory.CollectionStatus
-	if status := unraidDiskTemperatureStatus(host, disk); status.State == diskinventory.FieldUnavailable {
+	if status := unraidDiskTemperatureStatus(host, disk); status.State != "" {
 		collection = &diskinventory.CollectionStatus{Temperature: status}
 	}
 	resource := Resource{

internal/unifiedresources/registry.go

@@ -3732,6 +3732,7 @@ func (rr *ResourceRegistry) mergeInto(existing *Resource, incoming Resource, sou
 				incoming.PhysicalDisk.Collection,
 			)
 		}
+		pairPhysicalDiskTemperatureState(existing.PhysicalDisk, previous, incoming.PhysicalDisk)
 	}
 	if existing.PhysicalDisk != nil && (mergedPhysicalDisk || len(incoming.Incidents) > 0) {
 		existing.PhysicalDisk.Risk = physicalDiskRiskFromMeta(existing.PhysicalDisk, existing.Incidents)
@@ -4006,6 +4007,39 @@ func mergeTrueNASData(existing *TrueNASData, incoming *TrueNASData) *TrueNASData
 	return &merged
 }
 
+// pairPhysicalDiskTemperatureState stops one merge step presenting the shown
+// temperature under the other row's availability. The merge rules choose the
+// value by source preference and merge field states separately, so when the
+// two rows carry different temperatures the shown value can sit under the
+// other row's "available" state: a silent agent's retained reading beside a
+// Proxmox row that collected its own reading now, for example. When the
+// merged state says collected but the row the shown value came from says it
+// was not, the merged state becomes that row's own. Values never change, and a
+// state is only ever withdrawn here, never granted. previous may already be an
+// earlier step's merge, whose single state per field stands in for its rows.
+func pairPhysicalDiskTemperatureState(merged, previous, incoming *PhysicalDiskMeta) {
+	if merged == nil || previous == nil || incoming == nil || previous.Temperature == incoming.Temperature ||
+		!diskinventory.TemperatureCollected(merged.Temperature, merged.Collection) {
+		return
+	}
+	var shown *PhysicalDiskMeta
+	switch merged.Temperature {
+	case incoming.Temperature:
+		shown = incoming
+	case previous.Temperature:
+		shown = previous
+	default:
+		return
+	}
+	if diskinventory.TemperatureCollected(shown.Temperature, shown.Collection) {
+		return
+	}
+	if merged.Collection == nil {
+		merged.Collection = &diskinventory.CollectionStatus{}
+	}
+	merged.Collection.Temperature = shown.Collection.Temperature
+}
+
 func mergePhysicalDiskData(existing *PhysicalDiskMeta, incoming *PhysicalDiskMeta) *PhysicalDiskMeta {
 	if existing == nil {
 		return incoming

internal/unifiedresources/registry_merge_policy_test.go

@@ -634,3 +634,120 @@ func TestPhysicalDiskMergeFollowsAgentWithdrawalOfItsOwnReadings(t *testing.T) {
 		})
 	}
 }
+
+// A merged disk must never present its temperature under a state the
+// reading's own row does not claim. A silent agent's retained reading stays as
+// last-known context under the agent's own state, whatever the Proxmox row
+// collected or copied; a resumed agent's fresh reading stays collected over a
+// Proxmox copy of its earlier withdrawal; a standby withdrawal keeps the
+// pre-sleep reading as last-known.
+func TestPhysicalDiskMergePairsTemperatureWithItsCollectionState(t *testing.T) {
+	disk := func(temperature int, status diskinventory.FieldStatus) Resource {
+		return Resource{
+			Type: ResourceTypePhysicalDisk, Name: "WDC", Status: StatusOnline,
+			PhysicalDisk: &PhysicalDiskMeta{
+				DevPath: "/dev/sdb", Serial: "WD-PAIR1", Temperature: temperature,
+				Collection: &diskinventory.CollectionStatus{Temperature: status},
+			},
+		}
+	}
+	// The Proxmox row can also carry the SMART attributes copied from the
+	// agent's earlier report, which changes whose value the merge shows.
+	withSMART := func(r Resource) Resource {
+		powerOnHours := int64(1000)
+		r.PhysicalDisk.SMART = &SMARTMeta{PowerOnHours: &powerOnHours}
+		return r
+	}
+	identity := ResourceIdentity{MachineID: "WD-PAIR1", Hostnames: []string{"node1"}}
+	retained := diskinventory.Unavailable("host_agent", "host agent stopped reporting")
+	nodeSMART := diskinventory.Available("proxmox_node_smart")
+	smartctl := diskinventory.Available("smartctl")
+	withdrawn := diskinventory.Unavailable("smartctl", "host agent stopped reporting")
+	standby := diskinventory.Unavailable("smartctl", "disk is in standby")
+	noProxmoxTemp := diskinventory.Unsupported("proxmox_disks", "Proxmox disk inventory does not expose temperature")
+
+	for _, tc := range []struct {
+		name           string
+		agent, proxmox Resource
+		// wantTemperature 0 accepts either row's value: source preference,
+		// not this rule, decides which one is shown.
+		wantTemperature int
+		wantState       diskinventory.FieldStatus
+	}{
+		{"silent agent, Proxmox collected", disk(72, retained), disk(40, nodeSMART), 72, retained},
+		{"silent agent, no Proxmox reading", disk(72, retained), disk(0, noProxmoxTemp), 72, retained},
+		{"reporting agent, Proxmox collected", disk(41, smartctl), disk(40, nodeSMART), 41, smartctl},
+		{"silent agent, Proxmox copy of its earlier reading", disk(72, withdrawn), disk(41, smartctl), 72, withdrawn},
+		{"silent agent, Proxmox copy with SMART attributes", disk(72, withdrawn), withSMART(disk(41, smartctl)), 0, withdrawn},
+		{"resumed agent, Proxmox copy of its withdrawal", disk(50, smartctl), disk(41, withdrawn), 50, smartctl},
+		{"standby agent, Proxmox copy of its pre-sleep reading", disk(0, standby), disk(41, smartctl), 41, standby},
+	} {
+		for _, proxmoxFirst := range []bool{true, false} {
+			registry := NewRegistry(nil)
+			ingestProxmox := func() { registry.ingest(SourceProxmox, "pve1-node1-sdb", tc.proxmox, identity) }
+			ingestAgent := func() { registry.ingest(SourceAgent, "agent-1-sdb", tc.agent, identity) }
+			if proxmoxFirst {
+				ingestProxmox()
+				ingestAgent()
+			} else {
+				ingestAgent()
+				ingestProxmox()
+			}
+			disks := registry.ListByType(ResourceTypePhysicalDisk)
+			if len(disks) != 1 || disks[0].PhysicalDisk == nil {
+				t.Fatalf("%s (proxmox first=%v): expected one merged disk, got %+v", tc.name, proxmoxFirst, disks)
+			}
+			got := disks[0].PhysicalDisk
+			var state diskinventory.FieldStatus
+			if got.Collection != nil {
+				state = got.Collection.Temperature
+			}
+			valueOK := got.Temperature == tc.wantTemperature ||
+				(tc.wantTemperature == 0 && (got.Temperature == tc.agent.PhysicalDisk.Temperature || got.Temperature == tc.proxmox.PhysicalDisk.Temperature))
+			if !valueOK || state != tc.wantState {
+				t.Errorf("%s (proxmox first=%v): temperature=%d state=%+v, want %d with %+v",
+					tc.name, proxmoxFirst, got.Temperature, state, tc.wantTemperature, tc.wantState)
+			}
+		}
+	}
+}
+
+// Three rows for one disk with different values, none collected now: the
+// agent's SMART row withdrew its reading, the Unraid inventory row is past the
+// same lease, and the Proxmox row still carries a copy of the agent's earlier
+// "available" state. Whatever the ingest order, the merged disk must not
+// present a reading as collected.
+func TestPhysicalDiskMergeNeverPresentsAWithdrawnTemperatureAsCollected(t *testing.T) {
+	disk := func(temperature int, status diskinventory.FieldStatus) Resource {
+		return Resource{
+			Type: ResourceTypePhysicalDisk, Name: "WDC", Status: StatusOnline,
+			PhysicalDisk: &PhysicalDiskMeta{
+				DevPath: "/dev/sdb", Serial: "WD-THREE1", Temperature: temperature,
+				Collection: &diskinventory.CollectionStatus{Temperature: status},
+			},
+		}
+	}
+	identity := ResourceIdentity{MachineID: "WD-THREE1", Hostnames: []string{"tower"}}
+	rows := []struct {
+		source   DataSource
+		sourceID string
+		resource Resource
+	}{
+		{SourceAgent, "agent-1-sdb", disk(72, diskinventory.Unavailable("smartctl", "host agent stopped reporting"))},
+		{SourceAgent, "agent-1-unraid-sdb", disk(37, diskinventory.Unavailable("unraid", "host agent stopped reporting"))},
+		{SourceProxmox, "pve1-node1-sdb", disk(41, diskinventory.Available("smartctl"))},
+	}
+	for _, order := range [][]int{{0, 1, 2}, {0, 2, 1}, {1, 0, 2}, {1, 2, 0}, {2, 0, 1}, {2, 1, 0}} {
+		registry := NewRegistry(nil)
+		for _, i := range order {
+			registry.ingest(rows[i].source, rows[i].sourceID, rows[i].resource, identity)
+		}
+		disks := registry.ListByType(ResourceTypePhysicalDisk)
+		if len(disks) != 1 || disks[0].PhysicalDisk == nil {
+			t.Fatalf("order %v: expected one merged disk, got %d", order, len(disks))
+		}
+		if got := disks[0].PhysicalDisk; diskinventory.TemperatureCollected(got.Temperature, got.Collection) {
+			t.Errorf("order %v: withdrawn temperature presented as collected: temperature=%d collection=%+v", order, got.Temperature, got.Collection)
+		}
+	}
+}