From 01742e22798c692883a72f21eeb67efc81e4b20e Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Sun, 13 Sep 2026 18:25:42 +0100 Subject: [PATCH] fix(monitoring): preserve physical disk source identity on refresh Skipped disk polls must write the Proxmox source key back into inventory rather than hashing the canonical resource ID again. Add repeated-cycle runtime and JSON projection coverage for serial-less disks, distinct node/controller scopes and removal. Change-source: pulse-maintainer --- .../v6/internal/subsystems/agent-lifecycle.md | 17 +++ .../v6/internal/subsystems/monitoring.md | 17 +++ .../v6/internal/subsystems/registry.json | 2 + .../internal/subsystems/unified-resources.md | 17 +++ internal/monitoring/monitor.go | 8 +- .../physical_disk_roundtrip_test.go | 122 ++++++++++++++++++ internal/unifiedresources/views.go | 10 ++ 7 files changed, 192 insertions(+), 1 deletion(-) create mode 100644 internal/monitoring/physical_disk_roundtrip_test.go diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index 6d28977f19..04d5d36d3b 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -8039,3 +8039,20 @@ an isolated copy of the same test binary. Joining that probe's handler does not join upgraded handlers from preceding tests; process isolation keeps their final log writes from racing with the hook. The child retains race instrumentation and an explicit timeout, and the original server still fails the ordering assertion. + +### Physical-disk provider identity round trip + +The monitor's disk readback preserves the Proxmox source ID separately from +its canonical resource ID. Skipped physical-disk polls may refresh SMART data +but must not feed canonical hashes into source inventory, changing identity at +each subsequent projection. Views without Proxmox source metadata retain the +legacy ID fallback. This does not relax cross-host/device correlation, migrate +old duplicates, change agent authority, or redefine serial-less disk matching. + +`TestPhysicalDiskSkippedPollPreservesSourceIdentity` in +`internal/monitoring/physical_disk_roundtrip_test.go` drives eight real skipped +polls and adapter rebuilds for both path-shaped and bare device names, checking +provider keys, canonical IDs, JSON projection, temperature/size/cadence, separate +nodes/controller members, and confirmed inventory removal. +`TestPhysicalDiskReadbackSourceIDFallback` covers missing source metadata. +This is synthetic runtime evidence, not USB hardware or reporter acceptance. diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 9262174506..a0419bb6f3 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -4027,3 +4027,20 @@ and acknowledgement, requiring no queued notification. Existing routing, quiet-hours and cooldown tests now use real active manager occurrences rather than fabricated inactive alerts. This is callback-boundary proof, not a claim that already admitted or provider-accepted deliveries can be recalled. + +### Physical-disk provider identity round trip + +The monitor's disk readback preserves the Proxmox source ID separately from +its canonical resource ID. Skipped physical-disk polls may refresh SMART data +but must not feed canonical hashes into source inventory, changing identity at +each subsequent projection. Views without Proxmox source metadata retain the +legacy ID fallback. This does not relax cross-host/device correlation, migrate +old duplicates, change agent authority, or redefine serial-less disk matching. + +`TestPhysicalDiskSkippedPollPreservesSourceIdentity` in +`internal/monitoring/physical_disk_roundtrip_test.go` drives eight real skipped +polls and adapter rebuilds for both path-shaped and bare device names, checking +provider keys, canonical IDs, JSON projection, temperature/size/cadence, separate +nodes/controller members, and confirmed inventory removal. +`TestPhysicalDiskReadbackSourceIDFallback` covers missing source metadata. +This is synthetic runtime evidence, not USB hardware or reporter acceptance. diff --git a/docs/release-control/v6/internal/subsystems/registry.json b/docs/release-control/v6/internal/subsystems/registry.json index d405bc9a2b..e875eb0b81 100644 --- a/docs/release-control/v6/internal/subsystems/registry.json +++ b/docs/release-control/v6/internal/subsystems/registry.json @@ -1623,6 +1623,7 @@ "internal/monitoring/docker_metric_presence_test.go", "internal/monitoring/monitor_host_agent_removal_lifecycle_test.go", "internal/monitoring/monitor_host_agents_test.go", + "internal/monitoring/physical_disk_roundtrip_test.go", "scripts/installtests/agent_state_dir_lifecycle_test.go", "scripts/installtests/install_ps1_test.go", "scripts/installtests/install_sh_test.go" @@ -8218,6 +8219,7 @@ "internal/monitoring/docker_filesystem_evidence_test.go", "internal/monitoring/issue1595_collection_trust_test.go", "internal/monitoring/issue1913_host_continuity_test.go", + "internal/monitoring/physical_disk_roundtrip_test.go", "internal/unifiedresources/adapter_coverage_test.go", "internal/unifiedresources/adapters_test.go", "internal/unifiedresources/ceph_pool_health_contract_test.go", diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index a152c7a2d2..4c0aefd1a6 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -5214,3 +5214,20 @@ inference and presentation path, including preservation of the reciprocal link; `TestProxmoxOneWayLinkRejectsHostLocalEndpointCorroboration` protect management network and explicit report-IP compatibility. These synthetic cases do not establish the cause of a particular installation's association. + +### Physical-disk provider identity round trip + +The monitor's disk readback preserves the Proxmox source ID separately from +its canonical resource ID. Skipped physical-disk polls may refresh SMART data +but must not feed canonical hashes into source inventory, changing identity at +each subsequent projection. Views without Proxmox source metadata retain the +legacy ID fallback. This does not relax cross-host/device correlation, migrate +old duplicates, change agent authority, or redefine serial-less disk matching. + +`TestPhysicalDiskSkippedPollPreservesSourceIdentity` in +`internal/monitoring/physical_disk_roundtrip_test.go` drives eight real skipped +polls and adapter rebuilds for both path-shaped and bare device names, checking +provider keys, canonical IDs, JSON projection, temperature/size/cadence, separate +nodes/controller members, and confirmed inventory removal. +`TestPhysicalDiskReadbackSourceIDFallback` covers missing source metadata. +This is synthetic runtime evidence, not USB hardware or reporter acceptance. diff --git a/internal/monitoring/monitor.go b/internal/monitoring/monitor.go index b37b640209..49d0cf35bb 100644 --- a/internal/monitoring/monitor.go +++ b/internal/monitoring/monitor.go @@ -735,8 +735,14 @@ func physicalDiskFromReadStateView(view *unifiedresources.PhysicalDiskView) mode return models.PhysicalDisk{Wearout: unifiedresources.WearoutUnreported} } + // Preserve the provider key across the canonical read-state round trip. + // Older/synthetic views without source metadata retain their existing fallback. + sourceID := view.SourceID() + if sourceID == "" { + sourceID = view.ID() + } disk := models.PhysicalDisk{ - ID: view.ID(), + ID: sourceID, Node: view.Node(), Instance: view.Instance(), DevPath: view.DevPath(), diff --git a/internal/monitoring/physical_disk_roundtrip_test.go b/internal/monitoring/physical_disk_roundtrip_test.go new file mode 100644 index 0000000000..4f00d814ad --- /dev/null +++ b/internal/monitoring/physical_disk_roundtrip_test.go @@ -0,0 +1,122 @@ +package monitoring + +import ( + "context" + "encoding/json" + "testing" + "time" + + "github.com/rcourtman/pulse-go-rewrite/internal/config" + "github.com/rcourtman/pulse-go-rewrite/internal/models" + "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" +) + +// Exercise the real skipped-poll path, not just the ID helper: canonical views +// are converted back into source state and then re-ingested by the adapter. +func TestPhysicalDiskSkippedPollPreservesSourceIdentity(t *testing.T) { + for _, device := range []string{"/dev/sdx", "sdx"} { + t.Run(device, func(t *testing.T) { + state := models.NewState() + nodes := []models.Node{{ID: "pve-node1", Name: "node1", Instance: "pve"}, {ID: "pve-node2", Name: "node2", Instance: "pve"}} + state.UpdateNodesForInstance("pve", nodes) + var disks []models.PhysicalDisk + for _, fixture := range []struct{ node, target string }{{"node1", ""}, {"node2", ""}, {"node1", "megaraid,0"}, {"node1", "megaraid,1"}} { + disks = append(disks, models.PhysicalDisk{ + ID: unifiedresources.ProxmoxPhysicalDiskSourceID("pve", fixture.node, device, "", fixture.target), + Instance: "pve", Node: fixture.node, DevPath: device, Target: fixture.target, + Model: "USB fixture", Size: 1024, Temperature: 37, Wearout: -1, LastChecked: time.Now(), + ExpectedUpdateInterval: 5 * time.Minute, + }) + } + state.UpdatePhysicalDisks("pve", disks) + adapter := unifiedresources.NewMonitorAdapter(unifiedresources.NewRegistry(nil)) + adapter.PopulateFromSnapshot(state.GetSnapshot()) + m := &Monitor{state: state, resourceStore: adapter, lastPhysicalDiskPoll: map[string]time.Time{"pve": time.Now()}} + canonical := map[string]bool{} + for _, view := range adapter.PhysicalDisks() { + canonical[view.ID()] = true + } + if len(canonical) != len(disks) { + t.Fatalf("initial disks = %d, want %d", len(canonical), len(disks)) + } + for cycle := 0; cycle < 8; cycle++ { + m.maybePollPhysicalDisksAsync(context.Background(), "pve", &config.PVEInstance{}, nil, nil, nil, nil) + got := state.GetSnapshot().PhysicalDisks + if len(got) != len(disks) { + t.Fatalf("cycle %d: source count %d", cycle, len(got)) + } + wantIDs := map[string]bool{} + for _, disk := range disks { + wantIDs[disk.ID] = true + } + for _, disk := range got { + if !wantIDs[disk.ID] { + t.Fatalf("cycle %d: canonical ID leaked into provider state: %q", cycle, disk.ID) + } + if disk.DevPath != device || disk.Temperature != 37 || disk.Size != 1024 || disk.ExpectedUpdateInterval != 5*time.Minute { + t.Fatalf("cycle %d: metadata lost: %+v", cycle, disk) + } + } + adapter.PopulateFromSnapshot(state.GetSnapshot()) + views := adapter.PhysicalDisks() + if len(views) != len(disks) { + t.Fatalf("cycle %d: view count = %d", cycle, len(views)) + } + for _, view := range views { + if !canonical[view.ID()] { + t.Fatalf("cycle %d: canonical identity churn: %s", cycle, view.ID()) + } + } + // Retain the JSON-visible projection as well as the typed read-state checks. + payload, err := json.Marshal(adapter.GetAll()) + if err != nil { + t.Fatal(err) + } + var resources []unifiedresources.Resource + if err := json.Unmarshal(payload, &resources); err != nil { + t.Fatal(err) + } + count := 0 + for _, resource := range resources { + if resource.Type != unifiedresources.ResourceTypePhysicalDisk { + continue + } + count++ + if !canonical[resource.ID] || resource.Proxmox == nil || !wantIDs[resource.Proxmox.SourceID] || resource.PhysicalDisk == nil || resource.PhysicalDisk.Temperature != 37 { + t.Fatalf("cycle %d: JSON disk identity/metadata lost: %+v", cycle, resource) + } + } + if count != len(disks) { + t.Fatalf("cycle %d: JSON disk count %d", cycle, count) + } + } + // A confirmed full inventory removal must still remove source-owned disks. + state.UpdatePhysicalDisks("pve", nil) + adapter.PopulateFromSnapshot(state.GetSnapshot()) + if got := len(adapter.PhysicalDisks()); got != 0 { + t.Fatalf("removed inventory retained %d disks", got) + } + }) + } +} + +func TestPhysicalDiskReadbackSourceIDFallback(t *testing.T) { + for _, resource := range []unifiedresources.Resource{ + {ID: "canonical"}, + {ID: "canonical", Proxmox: &unifiedresources.ProxmoxData{}}, + {ID: "canonical", Proxmox: &unifiedresources.ProxmoxData{SourceID: " native "}}, + } { + view := unifiedresources.NewPhysicalDiskView(&resource) + want := "canonical" + if resource.Proxmox != nil && resource.Proxmox.SourceID != "" { + want = "native" + } + if got := physicalDiskFromReadStateView(&view).ID; got != want { + t.Fatalf("ID = %q, want %q", got, want) + } + } + var view unifiedresources.PhysicalDiskView + if view.SourceID() != "" { + t.Fatal("nil resource has a source ID") + } +} diff --git a/internal/unifiedresources/views.go b/internal/unifiedresources/views.go index b87f86f285..ff0a0b79a0 100644 --- a/internal/unifiedresources/views.go +++ b/internal/unifiedresources/views.go @@ -2120,6 +2120,16 @@ func (v PhysicalDiskView) ID() string { return v.r.ID } +// SourceID returns the Proxmox-native inventory key, not the canonical +// resource ID. Polling adapters must preserve this key when writing views back +// into provider state; hashing a canonical ID again creates a new resource. +func (v PhysicalDiskView) SourceID() string { + if v.r == nil || v.r.Proxmox == nil { + return "" + } + return strings.TrimSpace(v.r.Proxmox.SourceID) +} + func (v PhysicalDiskView) Name() string { if v.r == nil { return ""