From 5814c0a44cdffa958a7d43b7156d50c4c12ff3c9 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 13:35:59 +0100 Subject: [PATCH 1/9] Stop a failing availability check keeping a node online Once every source with a verdict had gone quiet, the stale pass counted any current availability sighting as online. A node that preserveOrExpireNodes expired stayed online while its HTTP check failed, and a quiet failing check lifted that node's offline verdict to warning. One sighting stands for every check on a target and carries whichever check was projected last, so a failing check that had just run also vouched for another check's old pass. Status aggregation now judges each check on a monitored resource by its own evidence window. A passing check whose evidence is still valid proves the resource answers, and reads online when no current source has a verdict. Every other check abstains, current or quiet: a failing check proves only that one port or service does not answer, so it never makes a guest count as stopped in the workloads summary or the Proxmox page. A check's own row keeps the ordinary rules, and a manual link judges the merged resource by the checks it carries. Projecting or retargeting a check now re-applies the stale pass's rule to targets whose status that pass owns, since the resources API replays checks after its stale pass and never runs another. Only a target's own quiet sources give the pass that ownership, never its checks' sighting. The poller now keeps each check's evidence tied to the observation that passed. It ages out a local location that stopped running beside other locations, as it already did for silent remote paths: the check took its freshness from the newest location, so a failing remote report kept an hour-old local pass counted as reachable with current evidence. An available check's evidence lasts only as long as its newest reachable path, and a remote result adopted as local after an entitlement lapse keeps the server's receipt time rather than the agent's clock. --- .../v6/internal/subsystems/monitoring.md | 15 +- .../internal/subsystems/unified-resources.md | 61 +++- internal/monitoring/availability_poller.go | 61 +++- .../monitoring/availability_probe_agent.go | 33 +- .../availability_probe_agent_test.go | 153 +++++++++ internal/unifiedresources/availability.go | 22 ++ internal/unifiedresources/registry.go | 68 +++- .../registry_merge_policy_test.go | 73 ++++- internal/unifiedresources/registry_test.go | 303 +++++++++++++++++- 9 files changed, 748 insertions(+), 41 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index bbd8faca71..2b8bf4c613 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -3028,7 +3028,20 @@ Monitoring retains one current status per target and location, using server receipt time for remote freshness while keeping the agent-authored observation time as evidence metadata. A stale or disconnected agent path derives as indeterminate at read time; slow or fast agent clocks cannot manufacture or -conceal a disconnect. Aggregation is conservative: all reachable paths are +conceal a disconnect. A local path beside other locations that has not run +within the same window derives as indeterminate too ("no recent local check"). +The target takes its freshness from its newest location, so otherwise a +failing remote report kept an old local pass counted as reachable and the +check read available with current evidence. For the same reason an available +check's evidence lasts only as long as its newest reachable path, not a +fresher path that failed. When the external-probe entitlement lapses, a +single remote result stands in for the local location until the local poller +runs, with the earlier of its agent check time and the server's receipt time +as its check time, so a fast agent clock cannot keep it current. An aggregate +over several locations never stands in: a target without a retained local +observation reads as never checked, and not connected, until the local poller +runs. +Aggregation is conservative: all reachable paths are healthy, mixed reachable and failed/unknown paths are degraded, all current paths unreachable are unavailable, and no reachable path with incomplete or indeterminate coverage is unknown. Only aggregate unavailability advances the diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index 53e49325b6..083816fbf9 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -6192,9 +6192,20 @@ pass and manual links both apply that function: the best of equal ones. A quiet source drops out of that decision. - Once every source is quiet, an `offline` verdict survives and any other verdict reads `warning`. The best of those wins. -- A current facet sighting carries no verdict (the PBS host-agent association, - an availability check projected onto its target). It counts as `online` only - when no current source has a verdict, and reads `warning` once quiet. +- A current facet sighting carries no verdict (the PBS host-agent association). + It counts as `online` only when no current source has a verdict, and reads + `warning` once quiet. +- Availability checks projected onto a monitored resource rank like that facet, + but their shared sighting is not their verdict. `availabilityChecksProveOnline` + judges each check at the pass's `now` by its own evidence window + (`Evidence.ValidUntil`: two poll intervals for a single local check, the + probe report window for a remote or multi-location one). An enabled, + passing check with current evidence proves the resource answers and reads + `online` once one of the resource's own sources has gone quiet and none that + is current has a verdict. Every other check abstains, current or quiet: a + failing check proves only that one port or service does not answer, and a + pass whose evidence lapsed proves nothing now. A check's own row keeps the ordinary + rules. The stale pass used to read delivery as the verdict. A Proxmox node the cluster reported offline on a live poll came back `online` once its linked agent fell @@ -6218,24 +6229,52 @@ online stays `online` when its linked agent stops reporting. A live agent keeps a node `online` when the Proxmox poll reports it offline, which the Proxmox nodes table reads with `connectionHealth: error` as a stale provider. When a guest's or container's only source goes quiet, a running one stays `warning` -and a stopped one stays `offline`. +and a stopped one stays `offline`. In each quiet case a projected availability +check with current evidence lifts the resource to `online`, as described +below. The sighting still reads `stale`, so health keeps its `telemetry_stale` reason and monitored-system reasons are unchanged. In-memory clones keep the verdicts, including a facet's missing one. A serialized copy carries none, so `IngestResources` gives each sighting of a copy that lost them all the stored resource status; a merged row's separate source verdicts do not survive that -round trip. A manual link that joins two resources reported by one source keeps +round trip, and a copy whose status a passing check supplied gives that status +to every source. A manual link that joins two resources reported by one source keeps the fresher sighting. `registry_merge_policy_test.go` pins the node, poller-expired, guest, round-trip, copy, agent, Docker, Kubernetes, mixed-source and manual-link cases, and the aggregation table. -Availability checks still count by delivery. With every source that has a -verdict quiet, a current check keeps its target `online` even when the check -fails, as it did before. Several checks share one sighting whose freshness -comes from whichever check was projected last, so an availability verdict -needs each check's own freshness, including local checks that miss their -cadence, and has to survive manual links. That is left to a follow-up. +Availability checks used to count by delivery too. With every source that +has a verdict quiet, a current failing check kept a node `preserveOrExpireNodes` +expired `online`, and a quiet one lifted it to `warning`. Several checks share +one sighting whose freshness comes from whichever check was projected last, so +a failing check that just ran could also vouch for another check's old pass. +Each check is now judged by its own evidence. A failing probe no longer holds +an expired node `online`, and it never makes a guest read stopped to the +consumers that count `offline` as stopped (the workloads summary, the Proxmox +page counts). The failure stays on the check's own row, which raises an +outage incident once the check confirms one (past its failure threshold, from +every location). A check observed from several locations relies on the poller +ageing out a location that stopped running and lasting an available check's +evidence only as long as its newest reachable path (monitoring.md), so one +location's fresh failure cannot lend its freshness to another location's old +pass. + +The verdict is read from the checks on the resource when status is +aggregated. Projecting, re-running or retargeting a check also re-applies the +stale pass's rule to every target it touches whose status that pass owns, +because a check is not a delivery from the target's own sources and the +resources API replays checks after its stale pass. The pass owns a status only +through the target's own sightings: the checks' sighting going quiet neither +hands it the target nor, once removed, takes the target away, so a target +whose own sources have not gone quiet keeps the status they gave it. A check +retargeted away therefore takes its verdict with it at once. A +manual link keeps only the primary's own checks, so the merged resource is +judged by the checks it shows and never by the linked resource's fresher +sighting. `registry_test.go` pins the expired-node and quiet-guest cases, both +projection orders, re-judging on check changes, ownership through the +target's own sightings and the manual link; the aggregation table pins the +check rules. ### Canonical object drawer hierarchy diff --git a/internal/monitoring/availability_poller.go b/internal/monitoring/availability_poller.go index 9f13bf861a..68b8e6a09e 100644 --- a/internal/monitoring/availability_poller.go +++ b/internal/monitoring/availability_poller.go @@ -321,8 +321,7 @@ func (m *Monitor) availabilityStatusSnapshotForTargets(targets []config.Availabi } if legacyLocationID == expected[0] || expected[0] == config.AvailabilityObservationLocationLocal { if expected[0] == config.AvailabilityObservationLocationLocal { - legacy.ProbeAgentID = "" - legacy.ProbeReportReceivedAt = time.Time{} + legacy = adoptAvailabilityStatusAsLocal(legacy) } locations[expected[0]] = legacy } @@ -330,13 +329,22 @@ func (m *Monitor) availabilityStatusSnapshotForTargets(targets []config.Availabi } if len(expected) == 1 && expected[0] == config.AvailabilityObservationLocationLocal { if _, ok := locations[expected[0]]; !ok { - if legacy, exists := out[target.ID]; exists { - legacy.ProbeAgentID = "" - legacy.ProbeReportReceivedAt = time.Time{} - locations[expected[0]] = legacy - } else { + legacy, exists := out[target.ID] + switch { + case !exists: delete(out, target.ID) continue + case legacy.ExpectedLocations <= 1: + locations[expected[0]] = adoptAvailabilityStatusAsLocal(legacy) + default: + // An aggregate over several locations is not one observation + // the local location can stand in for: its freshness may be + // one path's and its availability another's. The target + // reads as never checked until the local poller runs, with + // an entry of its own so connection health cannot fall back + // to the cached state from before the lapse. + out[target.ID] = availabilityStatusFromTarget(target) + continue } } } @@ -348,6 +356,8 @@ func (m *Monitor) availabilityStatusSnapshotForTargets(targets []config.Availabi } if status.ProbeAgentID != "" { status = m.deriveAvailabilityProbeStaleness(target, status, now) + } else if len(expected) > 1 { + status = deriveLocalAvailabilityObservationStaleness(target, status, now) } locations[locationID] = status } @@ -356,6 +366,21 @@ func (m *Monitor) availabilityStatusSnapshotForTargets(targets []config.Availabi return out } +// adoptAvailabilityStatusAsLocal presents a retained observation as the local +// location's once the check falls back to local observation, as it does when +// the external-probe entitlement lapses. A local location's check time is its +// freshness, but a remote observation's check time is the agent's clock, so it +// is held to the server's receipt time: a fast agent clock must not keep the +// adopted result current after its receipt is cleared. +func adoptAvailabilityStatusAsLocal(status AvailabilityProbeStatus) AvailabilityProbeStatus { + if !status.ProbeReportReceivedAt.IsZero() && status.LastChecked.After(status.ProbeReportReceivedAt) { + status.LastChecked = status.ProbeReportReceivedAt + } + status.ProbeAgentID = "" + status.ProbeReportReceivedAt = time.Time{} + return status +} + func (m *Monitor) RefreshAvailabilityTargets() { if m == nil { return @@ -986,6 +1011,12 @@ func availabilityEvidenceEnvelope( } freshnessAt := status.FreshnessTime() + if reachableAt := availabilityReachablePathFreshness(status); !reachableAt.IsZero() { + // An available check's claim rests on its reachable paths alone, so + // its evidence lasts as long as the newest of them, not as long as a + // fresher path that failed. + freshnessAt = reachableAt + } if freshnessAt.IsZero() { freshnessAt = observedAt } @@ -1028,6 +1059,22 @@ func availabilityEvidenceEnvelope( return &envelope } +// availabilityReachablePathFreshness returns the newest server-authored +// freshness among the current paths that reached an available check observed +// from several locations, and zero otherwise. +func availabilityReachablePathFreshness(status AvailabilityProbeStatus) time.Time { + if !status.Available || len(status.Locations) < 2 { + return time.Time{} + } + var latest time.Time + for _, location := range status.Locations { + if location.Available && !location.Stale && location.FreshnessAt.After(latest) { + latest = location.FreshnessAt + } + } + return latest +} + func availabilityResourceTags(target config.AvailabilityTarget) []string { tags := []string{"agentless"} if target.TargetKind != "" { diff --git a/internal/monitoring/availability_probe_agent.go b/internal/monitoring/availability_probe_agent.go index b5c0adbb9f..442a9e973a 100644 --- a/internal/monitoring/availability_probe_agent.go +++ b/internal/monitoring/availability_probe_agent.go @@ -23,6 +23,10 @@ const availabilityProbeStaleFloor = 5 * time.Minute // assigned agent stops reporting. const availabilityProbeStaleError = "no recent report from probe agent" +// availabilityLocalCheckStaleError explains a local observation that stopped +// running while the check's other locations kept reporting. +const availabilityLocalCheckStaleError = "no recent local check" + // ProbeAvailabilityResult is one availability observation reported by a remote // host agent that owns the target's execution. type ProbeAvailabilityResult struct { @@ -316,9 +320,34 @@ func (m *Monitor) availabilityProbeAssignmentReference(targetID, locationID stri return now } +// deriveLocalAvailabilityObservationStaleness ages out a local observation +// that stopped running when the check also has other locations. Alone, a +// local observation's check time is the check's freshness, so its evidence +// lapses with it. Beside other locations the check takes the newest +// location's freshness, so a local pass that stopped would otherwise keep the +// check available on the strength of a failing remote report. +func deriveLocalAvailabilityObservationStaleness( + target config.AvailabilityTarget, + status AvailabilityProbeStatus, + now time.Time, +) AvailabilityProbeStatus { + if status.LastChecked.IsZero() || !availabilityProbeReportIsStale(target, status.LastChecked, now) { + return status + } + status.Outcome = string(AvailabilityProbeIndeterminate) + status.Available = false + status.LastError = availabilityLocalCheckStaleError + status.LatencyMillis = 0 + return status +} + func availabilityProbeStatusIsStale(status AvailabilityProbeStatus) bool { - return status.Outcome == string(AvailabilityProbeIndeterminate) && - strings.EqualFold(strings.TrimSpace(status.LastError), availabilityProbeStaleError) + if status.Outcome != string(AvailabilityProbeIndeterminate) { + return false + } + lastError := strings.TrimSpace(status.LastError) + return strings.EqualFold(lastError, availabilityProbeStaleError) || + strings.EqualFold(lastError, availabilityLocalCheckStaleError) } func availabilityProbeReportIsStale(target config.AvailabilityTarget, lastChecked time.Time, now time.Time) bool { diff --git a/internal/monitoring/availability_probe_agent_test.go b/internal/monitoring/availability_probe_agent_test.go index c2ae3f1fde..67e4fb007b 100644 --- a/internal/monitoring/availability_probe_agent_test.go +++ b/internal/monitoring/availability_probe_agent_test.go @@ -165,6 +165,91 @@ func TestMultiLocationAvailabilityTreatsLapsedPathAsUnknownCoverage(t *testing.T } } +// The local twin of the lapsed-path case. A local observation that stopped +// running keeps its last outcome, and a check with several locations takes the +// newest location's freshness, so a failing edge report used to keep vouching +// for a local pass from an hour ago: the check read available with current +// evidence, which the registry takes as proof that its target answers. +func TestMultiLocationAvailabilityAgesOutALocalPathThatStopped(t *testing.T) { + target := config.NormalizeAvailabilityTarget(config.AvailabilityTarget{ + ID: "service", + Name: "Customer API", + Address: "api.service.local", + Protocol: config.AvailabilityProbeHTTPS, + Enabled: true, + PollIntervalSecs: 60, + FailureThreshold: 2, + ObservationLocationIDs: []string{config.AvailabilityObservationLocationLocal, config.AvailabilityAgentObservationLocationID("edge-1")}, + }) + monitor := newProbeAgentTestMonitor(t, target) + monitor.SetLicenseChecker(licenseWithExternalProbe(true)) + now := time.Now().UTC() + old := now.Add(-time.Hour) + monitor.applyAvailabilityObservation(target, "local-ok", old, 8*time.Millisecond, AvailabilityProbeReachable, nil, nil, "", time.Time{}) + for attempt := 0; attempt < 2; attempt++ { + at := now.Add(time.Duration(attempt) * time.Second) + monitor.applyAvailabilityObservation(target, "edge-fail-"+string(rune('a'+attempt)), at, 30*time.Millisecond, AvailabilityProbeUnreachable, context.DeadlineExceeded, nil, "edge-1", at) + } + + readAt := now.Add(2 * time.Second) + status := monitor.availabilityStatusSnapshotForTargets([]config.AvailabilityTarget{target}, readAt)[target.ID] + if status.Available || status.AggregateState != AvailabilityAggregateUnknown { + t.Fatalf("aggregate status = %+v, want unknown coverage rather than an available check", status) + } + if status.ReportingLocations != 1 || status.ExpectedLocations != 2 { + t.Fatalf("coverage = %d/%d, want the stopped local path excluded", status.ReportingLocations, status.ExpectedLocations) + } + local := status.Locations[0] + if local.LocationID != config.AvailabilityObservationLocationLocal || !local.Stale || + local.Outcome != string(AvailabilityProbeIndeterminate) || local.LastError != availabilityLocalCheckStaleError { + t.Fatalf("local location = %+v, want stale unknown evidence", local) + } + resource, _ := availabilityResourceFromTarget(target, status, "", readAt) + if resource.Availability.Available || resource.Status != unifiedresources.StatusWarning || len(resource.Incidents) != 0 { + t.Fatalf("resource = available %v status %q incidents %+v, want an unproven check and no outage", resource.Availability.Available, resource.Status, resource.Incidents) + } + + monitor.applyAvailabilityObservation(target, "local-ok-again", readAt, 9*time.Millisecond, AvailabilityProbeReachable, nil, nil, "", time.Time{}) + status = monitor.availabilityStatusSnapshotForTargets([]config.AvailabilityTarget{target}, readAt)[target.ID] + if !status.Available || status.AggregateState != AvailabilityAggregateDegraded || status.Locations[0].Stale { + t.Fatalf("aggregate status = %+v, want a current local pass to make the check available again", status) + } +} + +// An available check's evidence lasts as long as its newest reachable path. A +// registry that re-reads this generation later judges the check at its own +// time, so evidence anchored on a fresher failing path would keep proving the +// target answers after the only passing path had aged out. +func TestMultiLocationAvailableEvidenceLastsAsLongAsItsReachablePath(t *testing.T) { + target := config.NormalizeAvailabilityTarget(config.AvailabilityTarget{ + ID: "service", + Name: "Customer API", + Address: "api.service.local", + Protocol: config.AvailabilityProbeHTTPS, + Enabled: true, + PollIntervalSecs: 60, + FailureThreshold: 2, + ObservationLocationIDs: []string{config.AvailabilityObservationLocationLocal, config.AvailabilityAgentObservationLocationID("edge-1")}, + }) + monitor := newProbeAgentTestMonitor(t, target) + monitor.SetLicenseChecker(licenseWithExternalProbe(true)) + now := time.Now().UTC() + passedAt := now.Add(-4 * time.Minute) + monitor.applyAvailabilityObservation(target, "local-ok", passedAt, 8*time.Millisecond, AvailabilityProbeReachable, nil, nil, "", time.Time{}) + monitor.applyAvailabilityObservation(target, "edge-fail", now, 30*time.Millisecond, AvailabilityProbeUnreachable, context.DeadlineExceeded, nil, "edge-1", now) + + status := monitor.availabilityStatusSnapshotForTargets([]config.AvailabilityTarget{target}, now)[target.ID] + if !status.Available || status.AggregateState != AvailabilityAggregateDegraded { + t.Fatalf("aggregate status = %+v, want an available check with one failing path", status) + } + resource, _ := availabilityResourceFromTarget(target, status, "", now) + evidence := resource.Availability.Evidence + want := passedAt.Add(availabilityProbeStaleWindow(target)) + if evidence == nil || evidence.ValidUntil == nil || !evidence.ValidUntil.Equal(want) { + t.Fatalf("evidence = %+v, want it valid until %s, one window after the reachable path", evidence, want) + } +} + func licenseWithExternalProbe(enabled bool) func(string) bool { return func(feature string) bool { return enabled && feature == pkglicensing.FeatureExternalProbe @@ -237,6 +322,74 @@ func TestAvailabilityPollProviderSkipsProbeAssignedTargetsAndResumesOnLapse(t *t } } +// When the entitlement lapses, a remote check's last result stands in as the +// local location's until the local poller runs. A local location's check time +// is its freshness, so the agent's clock must not carry over: a result checked +// a day ahead would otherwise read current, and keep its target answering, for +// a day without a local execution. +func TestAvailabilityLapseAdoptsARemoteResultAtItsReceiptTime(t *testing.T) { + target := config.NormalizeAvailabilityTarget(probeAgentTarget("remote", "agent-1")) + monitor := newProbeAgentTestMonitor(t, target) + monitor.SetLicenseChecker(licenseWithExternalProbe(true)) + now := time.Now().UTC() + receivedAt := now.Add(-time.Minute) + agentClock := now.Add(24 * time.Hour) + monitor.applyAvailabilityObservation(target, "remote-ok", agentClock, 8*time.Millisecond, AvailabilityProbeReachable, nil, nil, "agent-1", receivedAt) + + monitor.SetLicenseChecker(licenseWithExternalProbe(false)) + status := monitor.availabilityStatusSnapshotForTargets([]config.AvailabilityTarget{target}, now)[target.ID] + if status.ProbeAgentID != "" || !status.LastChecked.Equal(receivedAt) { + t.Fatalf("adopted status = %+v, want a local result checked at the server receipt time", status) + } + resource, _ := availabilityResourceFromTarget(target, status, "", now) + evidence := resource.Availability.Evidence + want := receivedAt.Add(2 * time.Duration(target.EffectivePollIntervalSecs()) * time.Second) + if evidence == nil || evidence.ValidUntil == nil || !evidence.ValidUntil.Equal(want) { + t.Fatalf("evidence = %+v, want it valid until %s", evidence, want) + } +} + +// The stored result of a check with several locations is an aggregate, whose +// freshness may be one path's and whose availability another's. After a lapse +// it must not stand in for the local location: one path passing an hour ago +// and another failing now would read as a current local pass. +func TestAvailabilityLapseNeverAdoptsAMultiLocationAggregate(t *testing.T) { + target := config.NormalizeAvailabilityTarget(config.AvailabilityTarget{ + ID: "service", + Name: "Customer API", + Address: "api.service.local", + Protocol: config.AvailabilityProbeHTTPS, + Enabled: true, + PollIntervalSecs: 60, + FailureThreshold: 2, + ObservationLocationIDs: []string{ + config.AvailabilityAgentObservationLocationID("edge-1"), + config.AvailabilityAgentObservationLocationID("edge-2"), + }, + }) + monitor := newProbeAgentTestMonitor(t, target) + monitor.SetLicenseChecker(licenseWithExternalProbe(true)) + now := time.Now().UTC() + passedAt := now.Add(-time.Hour) + monitor.applyAvailabilityObservation(target, "edge-1-ok", passedAt, 8*time.Millisecond, AvailabilityProbeReachable, nil, nil, "edge-1", passedAt) + monitor.applyAvailabilityObservation(target, "edge-2-fail", now, 30*time.Millisecond, AvailabilityProbeUnreachable, context.DeadlineExceeded, nil, "edge-2", now) + + monitor.SetLicenseChecker(licenseWithExternalProbe(false)) + status, ok := monitor.availabilityStatusSnapshotForTargets([]config.AvailabilityTarget{target}, now)[target.ID] + if !ok || status.Available || !status.LastChecked.IsZero() || status.AggregateState != "" { + t.Fatalf("status after the lapse = %+v (present %v), want a never-checked target", status, ok) + } + if resource, _ := availabilityResourceFromTarget(target, status, "", now); resource.Status != unifiedresources.StatusUnknown { + t.Fatalf("check row status = %q, want unknown like any never-checked target", resource.Status) + } + // An absent entry would let connection health fall back to the last + // cached state, which still says connected. + connected, reported := availabilityPollProvider{}.ConnectionStatuses(monitor)[availabilityConnectionKey(target.ID)] + if !reported || connected { + t.Fatalf("connection status = %v (reported %v), want not connected", connected, reported) + } +} + func TestRefreshAvailabilityTargetsUnschedulesProbeAssignedTargets(t *testing.T) { monitor := newProbeAgentTestMonitor(t, probeAgentTarget("remote", "agent-1"), diff --git a/internal/unifiedresources/availability.go b/internal/unifiedresources/availability.go index 35620d04da..6eaec642dc 100644 --- a/internal/unifiedresources/availability.go +++ b/internal/unifiedresources/availability.go @@ -4,6 +4,9 @@ import ( "fmt" "sort" "strings" + "time" + + "github.com/rcourtman/pulse-go-rewrite/internal/operationaltrust" ) // AvailabilityChecksForResource returns the complete canonical availability @@ -28,6 +31,25 @@ func AvailabilityCheckByTargetID(resource Resource, targetID string) *Availabili return nil } +// availabilityChecksProveOnline reports whether any availability check +// projected onto a monitored resource proves, at now, that the resource +// answers. Only a passing check with current evidence does. Each check is +// judged by its own evidence window (two poll intervals for a local check, +// the report window for a remote probe), so one check's recent run cannot +// vouch for another check's old pass, and a local check that missed its +// cadence keeps Available but proves nothing. A failing check proves only +// that one port or service does not answer, so it never decides the +// resource's status at all: not online, offline or warning, current or quiet. +func availabilityChecksProveOnline(checks []AvailabilityData, now time.Time) bool { + for _, check := range checks { + if check.Enabled && check.Available && check.Evidence != nil && + check.Evidence.FreshnessAt(now) == operationaltrust.EvidenceFresh { + return true + } + } + return false +} + func normalizeResourceAvailability(resource *Resource) { if resource == nil { return diff --git a/internal/unifiedresources/registry.go b/internal/unifiedresources/registry.go index 6c9a786cad..12e4ca842b 100644 --- a/internal/unifiedresources/registry.go +++ b/internal/unifiedresources/registry.go @@ -1799,16 +1799,16 @@ func (rr *ResourceRegistry) markStaleLocked(now time.Time, thresholds map[DataSo changed = changed || status.Status != "stale" status.Status = "stale" resource.SourceStatus[source] = status - staleFound = true + // The sighting of checks projected onto a resource is not one + // of its own sources, so its going quiet alone hands the stale + // pass nothing to decide; see reapplyStaleStatusForChecks. + if source != SourceAvailability || isAvailabilityOwnedResource(*resource) { + staleFound = true + } } } if staleFound { - recomputed := aggregateStatus(resource) - if recomputed != StatusUnknown { - resource.Status = recomputed - } else if resource.Status == StatusOnline { - resource.Status = StatusWarning - } + applyStaleStatus(resource, now) } changed = changed || resource.Status != previousStatus } @@ -1817,6 +1817,34 @@ func (rr *ResourceRegistry) markStaleLocked(now time.Time, thresholds map[DataSo } } +// applyStaleStatus is the stale pass's status rule for a resource with a +// quiet sighting. +func applyStaleStatus(resource *Resource, now time.Time) { + recomputed := aggregateStatus(resource, now) + if recomputed != StatusUnknown { + resource.Status = recomputed + } else if resource.Status == StatusOnline { + resource.Status = StatusWarning + } +} + +// reapplyStaleStatusForChecks re-applies the stale pass's rule to a resource +// whose availability checks just changed, when that pass owns its status +// because one of the resource's own sources went quiet. Projecting or +// retargeting a check is not a delivery from those sources, and the resources +// API replays checks after its stale pass, so the verdict the old checks gave +// would otherwise stand. The checks' own sighting never confers ownership: +// a resource whose own sources have not gone quiet keeps the status they gave +// it, whatever its checks say. +func reapplyStaleStatusForChecks(resource *Resource, now time.Time) { + for source, sighting := range resource.SourceStatus { + if source != SourceAvailability && sighting.Status == "stale" { + applyStaleStatus(resource, now) + return + } + } +} + func (rr *ResourceRegistry) ingestProxmoxNode(node models.Node, linkedHost *models.Host) { resource, identity := resourceFromProxmoxNode(node, linkedHost) rr.ingest(SourceProxmox, node.ID, resource, identity) @@ -3035,6 +3063,7 @@ func (rr *ResourceRegistry) projectAvailabilityCheckLocked( Status: sourceSightingStatus(checkResource.LastSeen), LastSeen: checkResource.LastSeen, } + reapplyStaleStatusForChecks(target, time.Now().UTC()) } func (rr *ResourceRegistry) removeAvailabilityProjectionLocked( @@ -3069,6 +3098,7 @@ func (rr *ResourceRegistry) removeAvailabilityProjectionLocked( resource.Sources = removeDataSource(resource.Sources, SourceAvailability) delete(resource.SourceStatus, SourceAvailability) } + reapplyStaleStatusForChecks(resource, time.Now().UTC()) } // Build a fresh slice rather than compacting in place: callers may hold @@ -4766,7 +4796,7 @@ func (rr *ResourceRegistry) mergeResourceData(primary *Resource, other *Resource // Manual links combine already-normalized resources. Preserve each metric's // recorded source instead of flattening the linked resource to SourceAgent. primary.Metrics = mergeMetrics(primary, primary.Metrics, other.Metrics, "", time.Now().UTC(), primary.SourceStatus, thresholds) - primary.Status = aggregateStatus(primary) + primary.Status = aggregateStatus(primary, time.Now().UTC()) } func (rr *ResourceRegistry) updateSourceMappings(fromID, toID string) { @@ -6093,15 +6123,25 @@ func chooseStatus(existing ResourceStatus, incoming ResourceStatus, source DataS // order: the highest-priority verdict, the best of equal ones. A source that // went quiet drops out of that decision, so a node the cluster reports // offline stays offline when its linked agent falls silent. A current facet -// sighting without a verdict (the PBS association, an availability check) -// counts as online only when no current source has one. Once every source is -// quiet, an offline verdict survives (a node the poller expired, an agent past -// its lease) and any other reads as warning; the best of those wins. -func aggregateStatus(resource *Resource) ResourceStatus { +// sighting without a verdict (the PBS association) counts as online only when +// no current source has one. Availability checks projected onto a monitored +// resource rank the same way, but their sighting is not their verdict: one +// sighting stands for every check, so each check is judged at now by its own +// evidence (see availabilityChecksProveOnline), and a check that does not +// prove the resource answers abstains. Checks only decide once one of the +// resource's own sources has gone quiet, so the stale pass owns the status and +// reverts it when the evidence lapses. Once every source is quiet, an +// offline verdict survives (a node the poller expired, an agent past its +// lease) and any other reads as warning; the best of those wins. +func aggregateStatus(resource *Resource, now time.Time) ResourceStatus { var current, quiet ResourceStatus currentPriority := -1 deliveredWithoutVerdict := false + checkedTarget := !isAvailabilityOwnedResource(*resource) for source, sighting := range resource.SourceStatus { + if source == SourceAvailability && checkedTarget { + continue + } switch strings.ToLower(strings.TrimSpace(sighting.Status)) { case "online": switch sighting.reported { @@ -6129,6 +6169,8 @@ func aggregateStatus(resource *Resource) ResourceStatus { return current case deliveredWithoutVerdict: return StatusOnline + case checkedTarget && quiet != "" && availabilityChecksProveOnline(AvailabilityChecksForResource(*resource), now): + return StatusOnline case quiet != "": return quiet default: diff --git a/internal/unifiedresources/registry_merge_policy_test.go b/internal/unifiedresources/registry_merge_policy_test.go index 5b7a92adb6..1250979121 100644 --- a/internal/unifiedresources/registry_merge_policy_test.go +++ b/internal/unifiedresources/registry_merge_policy_test.go @@ -711,18 +711,30 @@ func TestIngestResourcesKeepsAFacetSightingWithoutAVerdict(t *testing.T) { // aggregateStatus is the one rule the stale pass and manual links apply. // Current sources decide by their own verdicts in chooseStatus's priority -// order; quiet sources only decide when no source is current. +// order; quiet sources only decide when no source is current. Availability +// checks on a monitored resource are judged by their own evidence, not their +// shared sighting. func TestAggregateStatusReadsSourceVerdictsNotDelivery(t *testing.T) { + now := time.Now().UTC() + lapsed := now.Add(-10 * time.Minute) current := func(reported ResourceStatus) SourceStatus { return SourceStatus{Status: "online", reported: reported} } quiet := func(reported ResourceStatus) SourceStatus { return SourceStatus{Status: "stale", reported: reported} } + check := func(passing bool, checkedAt time.Time) []AvailabilityData { + return []AvailabilityData{{ + TargetID: "probe-1", Enabled: true, Available: passing, LastChecked: &checkedAt, + Evidence: availabilityProbeEvidence(t, "probe-1", checkedAt), + }} + } for _, tc := range []struct { - name string - sightings map[DataSource]SourceStatus - want ResourceStatus + name string + resourceType ResourceType + sightings map[DataSource]SourceStatus + checks []AvailabilityData + want ResourceStatus }{ { name: "live poll reports offline, agent quiet", @@ -766,9 +778,57 @@ func TestAggregateStatusReadsSourceVerdictsNotDelivery(t *testing.T) { }, { name: "a facet without a verdict never outranks a current verdict", - sightings: map[DataSource]SourceStatus{SourceProxmox: current(StatusOffline), SourceAvailability: current("")}, + sightings: map[DataSource]SourceStatus{SourceProxmox: current(StatusOffline), SourcePBS: current("")}, want: StatusOffline, }, + { + name: "a current failing check abstains", + sightings: map[DataSource]SourceStatus{SourceProxmox: quiet(StatusOffline), SourceAvailability: current("")}, + checks: check(false, now), + want: StatusOffline, + }, + { + name: "a quiet failing check abstains rather than lifting an offline verdict to warning", + sightings: map[DataSource]SourceStatus{SourceProxmox: quiet(StatusOffline), SourceAvailability: quiet("")}, + checks: check(false, lapsed), + want: StatusOffline, + }, + { + name: "a passing check whose evidence lapsed abstains under a current sighting", + sightings: map[DataSource]SourceStatus{SourceProxmox: quiet(StatusOnline), SourceAvailability: current("")}, + checks: check(true, lapsed), + want: StatusWarning, + }, + { + name: "a passing check with current evidence proves the target answers under a quiet sighting", + sightings: map[DataSource]SourceStatus{SourceProxmox: quiet(StatusOffline), SourceAvailability: quiet("")}, + checks: check(true, now), + want: StatusOnline, + }, + { + name: "a passing check never lifts a resource whose own sources have not gone quiet", + sightings: map[DataSource]SourceStatus{SourceProxmox: {Status: "unknown"}, SourceAvailability: current("")}, + checks: check(true, now), + want: StatusUnknown, + }, + { + name: "a passing check never outranks a current verdict", + sightings: map[DataSource]SourceStatus{SourceProxmox: current(StatusOffline), SourceAvailability: current("")}, + checks: check(true, now), + want: StatusOffline, + }, + { + name: "a check's own row keeps its quiet verdict", + resourceType: ResourceTypeNetworkEndpoint, + sightings: map[DataSource]SourceStatus{SourceAvailability: quiet(StatusOffline)}, + want: StatusOffline, + }, + { + name: "a check's own row keeps its current verdict", + resourceType: ResourceTypeNetworkEndpoint, + sightings: map[DataSource]SourceStatus{SourceAvailability: current(StatusWarning)}, + want: StatusWarning, + }, { name: "current unknown verdict leaves the quiet sources to decide", sightings: map[DataSource]SourceStatus{SourceProxmox: current(StatusUnknown), SourceAgent: quiet(StatusOnline)}, @@ -781,7 +841,8 @@ func TestAggregateStatusReadsSourceVerdictsNotDelivery(t *testing.T) { }, } { t.Run(tc.name, func(t *testing.T) { - if got := aggregateStatus(&Resource{SourceStatus: tc.sightings}); got != tc.want { + resource := &Resource{Type: tc.resourceType, SourceStatus: tc.sightings, AvailabilityChecks: tc.checks} + if got := aggregateStatus(resource, now); got != tc.want { t.Fatalf("aggregateStatus = %q, want %q", got, tc.want) } }) diff --git a/internal/unifiedresources/registry_test.go b/internal/unifiedresources/registry_test.go index 7c98c19bca..53214cd891 100644 --- a/internal/unifiedresources/registry_test.go +++ b/internal/unifiedresources/registry_test.go @@ -5686,7 +5686,8 @@ func TestMarkStaleRecomputesFromRemainingFreshSources(t *testing.T) { Availability: &AvailabilityData{ TargetID: "probe-delly", LinkedResourceID: host.ID, Address: "192.0.2.5", Protocol: "tcp", Port: 8006, - Enabled: true, Available: true, + Enabled: true, Available: true, LastChecked: &recentNow, + Evidence: availabilityProbeEvidence(t, "probe-delly", recentNow), }, }, Identity: ResourceIdentity{IPAddresses: []string{"192.0.2.5"}}, @@ -5759,6 +5760,7 @@ func TestMarkStaleKeepsProxmoxRuntimeStateIndependentOfAvailabilityFacet(t *test Availability: &AvailabilityData{ TargetID: "probe-102", LinkedResourceID: checkedID, Address: "192.0.2.102", Protocol: "icmp", Enabled: true, Available: true, + LastChecked: &freshSeen, Evidence: availabilityProbeEvidence(t, "probe-102", freshSeen), }, }, }}) @@ -5781,6 +5783,305 @@ func TestMarkStaleKeepsProxmoxRuntimeStateIndependentOfAvailabilityFacet(t *test } } +// availabilityCheckRecord is a check the poller ran at checkedAt against the +// resource targetID names. Its evidence stays current for two minutes, the +// two poll intervals of a local check; Available stays as the last run left +// it, as a local check that misses its cadence does. +func availabilityCheckRecord(t *testing.T, checkID, targetID string, passing bool, checkedAt time.Time) IngestRecord { + t.Helper() + status, failures := StatusOnline, 0 + if !passing { + status, failures = StatusOffline, 3 + } + return IngestRecord{ + SourceID: checkID, + Resource: Resource{ + Type: ResourceTypeNetworkEndpoint, Name: checkID, Status: status, LastSeen: checkedAt, + Sources: []DataSource{SourceAvailability}, + Availability: &AvailabilityData{ + TargetID: checkID, LinkedResourceID: targetID, + Address: "192.0.2.80", Protocol: "http", Port: 443, + Enabled: true, Available: passing, LastChecked: &checkedAt, + ConsecutiveFailures: failures, FailureThreshold: 2, PollIntervalSeconds: 60, + Evidence: availabilityProbeEvidence(t, checkID, checkedAt), + }, + }, + } +} + +func ingestProxmoxGuestSeenAt(t *testing.T, rr *ResourceRegistry, sourceID string, seen time.Time) string { + t.Helper() + rr.IngestRecords(SourceProxmox, []IngestRecord{{ + SourceID: sourceID, + Resource: Resource{ + Type: ResourceTypeSystemContainer, Name: sourceID, Status: StatusOnline, LastSeen: seen, + Proxmox: &ProxmoxData{RuntimeStatus: "running", NodeName: "node-a"}, + }, + }}) + for _, resource := range rr.ListByType(ResourceTypeSystemContainer) { + if resource.Name == sourceID { + return resource.ID + } + } + t.Fatalf("guest %s not ingested", sourceID) + return "" +} + +// A failing availability check proves only that one port or service does not +// answer, so it never decides its target's status: a node the poller expired +// stays offline, a guest whose poll went quiet stays a warning instead of +// counting as stopped, and neither reads online. Only a passing check with +// current evidence proves the target answers. The stale pass used to count +// any current check as online, so a failing probe kept an expired node up. +func TestStalePassReadsAvailabilityCheckVerdicts(t *testing.T) { + now := time.Now().UTC() + quietSeen := now.Add(-5 * time.Minute) + lapsed := now.Add(-10 * time.Minute) + expiredNode := models.StateSnapshot{Nodes: []models.Node{{ + ID: "homelab-pve9", Name: "pve9", Instance: "homelab", ClusterName: "homelab", + Status: "offline", LastSeen: quietSeen, + }}} + quietGuest := models.StateSnapshot{VMs: []models.VM{{ + ID: "homelab:pve9:101", Name: "web", Node: "pve9", Instance: "homelab", + VMID: 101, Status: "running", Type: "qemu", LastSeen: quietSeen, + }}} + for _, tc := range []struct { + name string + snapshot models.StateSnapshot + targetType ResourceType + passing bool + checkedAt time.Time + want ResourceStatus + }{ + {"expired node, current failing check", expiredNode, ResourceTypeAgent, false, now, StatusOffline}, + {"expired node, lapsed failing check", expiredNode, ResourceTypeAgent, false, lapsed, StatusOffline}, + {"expired node, lapsed passing check", expiredNode, ResourceTypeAgent, true, lapsed, StatusOffline}, + {"expired node, current passing check", expiredNode, ResourceTypeAgent, true, now, StatusOnline}, + {"quiet guest, current failing check", quietGuest, ResourceTypeVM, false, now, StatusWarning}, + {"quiet guest, lapsed failing check", quietGuest, ResourceTypeVM, false, lapsed, StatusWarning}, + {"quiet guest, current passing check", quietGuest, ResourceTypeVM, true, now, StatusOnline}, + } { + t.Run(tc.name, func(t *testing.T) { + rr := NewRegistry(nil) + rr.IngestSnapshot(tc.snapshot) + target := onlyResourceOfType(t, rr, tc.targetType) + rr.IngestRecords(SourceAvailability, []IngestRecord{ + availabilityCheckRecord(t, "probe-1", target.ID, tc.passing, tc.checkedAt), + }) + rr.MarkStale(now, nil) + + got, ok := rr.Get(target.ID) + if !ok || len(AvailabilityChecksForResource(*got)) != 1 { + t.Fatalf("check not projected onto %s: %+v", target.ID, got.AvailabilityChecks) + } + if got.Status != tc.want { + t.Fatalf("status = %q, want %q", got.Status, tc.want) + } + }) + } +} + +// One availability sighting stands for every check on a target, and it +// carries whichever check was projected last. Each check is judged by its own +// evidence instead, so a failing check that just ran cannot lend its +// freshness to another check's old pass, in either projection order. +func TestAvailabilityChecksAreJudgedByTheirOwnEvidence(t *testing.T) { + now := time.Now().UTC() + lapsed := now.Add(-10 * time.Minute) + for _, tc := range []struct { + name string + passedAt, failedAt time.Time + want ResourceStatus + }{ + {name: "lapsed pass, current failure", passedAt: lapsed, failedAt: now, want: StatusWarning}, + {name: "current pass, lapsed failure", passedAt: now, failedAt: lapsed, want: StatusOnline}, + } { + for _, order := range [][]string{{"pass", "fail"}, {"fail", "pass"}} { + t.Run(tc.name+", "+strings.Join(order, " then "), func(t *testing.T) { + rr := NewRegistry(nil) + target := ingestProxmoxGuestSeenAt(t, rr, "lab:node-a:101", now.Add(-5*time.Minute)) + records := map[string]IngestRecord{ + "pass": availabilityCheckRecord(t, "probe-pass", target, true, tc.passedAt), + "fail": availabilityCheckRecord(t, "probe-fail", target, false, tc.failedAt), + } + for _, key := range order { + rr.IngestRecords(SourceAvailability, []IngestRecord{records[key]}) + } + rr.MarkStale(now, nil) + + got, ok := rr.Get(target) + if !ok { + t.Fatal("target missing") + } + if checks := AvailabilityChecksForResource(*got); len(checks) != 2 { + t.Fatalf("expected both checks on the target, got %+v", checks) + } + if got.Status != tc.want { + t.Fatalf("status = %q, want %q", got.Status, tc.want) + } + }) + } + } +} + +// A check's verdict moves with the check. Projecting, re-running or +// retargeting a check is not a delivery from the resource's own sources, so a +// target whose status the stale pass owns is judged again at once rather than +// keeping the old checks' verdict until the next pass; the resources API +// replays checks after its stale pass and never runs another. +func TestChangedAvailabilityChecksJudgeTheirTargetsAgain(t *testing.T) { + now := time.Now().UTC() + quietSeen := now.Add(-5 * time.Minute) + rr := NewRegistry(nil) + first := ingestProxmoxGuestSeenAt(t, rr, "lab:node-a:101", quietSeen) + second := ingestProxmoxGuestSeenAt(t, rr, "lab:node-a:102", quietSeen) + rr.MarkStale(now, nil) + + status := func(id string) ResourceStatus { + t.Helper() + resource, ok := rr.Get(id) + if !ok { + t.Fatalf("resource %s missing", id) + } + return resource.Status + } + + rr.IngestRecords(SourceAvailability, []IngestRecord{ + availabilityCheckRecord(t, "probe-moving", first, true, now), + availabilityCheckRecord(t, "probe-failing", first, false, now), + }) + if got := status(first); got != StatusOnline { + t.Fatalf("first status = %q once a passing check lands, want online", got) + } + + rr.IngestRecords(SourceAvailability, []IngestRecord{ + availabilityCheckRecord(t, "probe-moving", second, true, now), + }) + left, _ := rr.Get(first) + if checks := AvailabilityChecksForResource(*left); len(checks) != 1 || checks[0].TargetID != "probe-failing" { + t.Fatalf("first checks = %+v, want only probe-failing", checks) + } + if got := status(first); got != StatusWarning { + t.Fatalf("first status = %q after its passing check moved away, want warning", got) + } + if got := status(second); got != StatusOnline { + t.Fatalf("second status = %q after the passing check moved onto it, want online", got) + } + + rr.IngestRecords(SourceAvailability, []IngestRecord{ + availabilityCheckRecord(t, "probe-moving", second, false, now), + }) + if got := status(second); got != StatusWarning { + t.Fatalf("second status = %q once its only check fails, want warning", got) + } + + rr.MarkStale(now, nil) + if got := status(first); got != StatusWarning { + t.Fatalf("first status = %q after the next pass, want warning", got) + } + if got := status(second); got != StatusWarning { + t.Fatalf("second status = %q after the next pass, want warning", got) + } +} + +// A projected check's sighting is not one of its target's own sources. Its +// going quiet hands the stale pass nothing to decide, and replacing the check +// takes no ownership away, so a target whose own source has never delivered +// keeps the status that source gave it whatever its checks say. +func TestAvailabilitySightingNeverHandsTheStalePassItsTarget(t *testing.T) { + now := time.Now().UTC() + quickChecks := map[DataSource]time.Duration{SourceAvailability: 30 * time.Second} + for _, tc := range []struct { + name string + ingested ResourceStatus + passing bool + }{ + {name: "online guest, quiet failing check", ingested: StatusOnline, passing: false}, + {name: "offline placeholder, quiet passing check", ingested: StatusOffline, passing: true}, + } { + t.Run(tc.name, func(t *testing.T) { + rr := NewRegistry(nil) + rr.IngestRecords(SourceProxmox, []IngestRecord{{ + SourceID: "lab:node-a:101", + Resource: Resource{ + Type: ResourceTypeSystemContainer, Name: "lab:node-a:101", Status: tc.ingested, + Proxmox: &ProxmoxData{RuntimeStatus: "running", NodeName: "node-a"}, + }, + }}) + target := onlyResourceOfType(t, rr, ResourceTypeSystemContainer).ID + + rr.IngestRecords(SourceAvailability, []IngestRecord{ + availabilityCheckRecord(t, "probe-1", target, tc.passing, now.Add(-time.Minute)), + }) + rr.MarkStale(now, quickChecks) + got, _ := rr.Get(target) + if sighting := got.SourceStatus[SourceAvailability]; sighting.Status != "stale" { + t.Fatalf("availability sighting = %+v, want stale", sighting) + } + if got.Status != tc.ingested { + t.Fatalf("status = %q with a quiet check, want %q from the target's own source", got.Status, tc.ingested) + } + + rr.IngestRecords(SourceAvailability, []IngestRecord{ + availabilityCheckRecord(t, "probe-1", target, !tc.passing, now), + }) + rr.MarkStale(now, quickChecks) + if got, _ := rr.Get(target); got.Status != tc.ingested { + t.Fatalf("status = %q after the check flipped, want %q", got.Status, tc.ingested) + } + }) + } +} + +// A manual link keeps the primary's own availability checks and not the +// linked resource's, so the merged resource is judged by the checks it +// carries. The linked resource's fresher availability sighting survives the +// merge as delivery bookkeeping; it must not become a verdict for checks the +// merged resource does not show. +func TestManualLinkJudgesOnlyTheChecksTheMergedResourceCarries(t *testing.T) { + now := time.Now().UTC() + quietSeen := now.Add(-5 * time.Minute) + store := NewMemoryStore() + if err := store.AddLink(ResourceLink{ResourceA: "vm-web", ResourceB: "agent-web", PrimaryID: "vm-web"}); err != nil { + t.Fatalf("add link: %v", err) + } + rr := NewRegistry(store) + rr.IngestResources([]Resource{ + { + ID: "vm-web", Type: ResourceTypeVM, Name: "web", Status: StatusWarning, LastSeen: quietSeen, + Sources: []DataSource{SourceProxmox}, + SourceStatus: map[DataSource]SourceStatus{ + SourceProxmox: {Status: "stale", LastSeen: quietSeen, reported: StatusOnline}, + }, + }, + { + ID: "agent-web", Type: ResourceTypeAgent, Name: "web", Status: StatusWarning, LastSeen: now, + Sources: []DataSource{SourceAgent, SourceAvailability}, + SourceStatus: map[DataSource]SourceStatus{ + SourceAgent: {Status: "stale", LastSeen: quietSeen, reported: StatusOnline}, + SourceAvailability: {Status: "online", LastSeen: now}, + }, + AvailabilityChecks: []AvailabilityData{{ + TargetID: "probe-web", Enabled: true, Available: false, LastChecked: &now, + ConsecutiveFailures: 3, FailureThreshold: 2, + Evidence: availabilityProbeEvidence(t, "probe-web", now), + }}, + Agent: &AgentData{AgentID: "host-web", Hostname: "web"}, + }, + }) + + merged, ok := rr.Get("vm-web") + if !ok { + t.Fatal("linked resource missing") + } + if checks := AvailabilityChecksForResource(*merged); len(checks) != 0 { + t.Fatalf("merged resource exposes the linked resource's checks: %+v", checks) + } + if merged.Status != StatusWarning { + t.Fatalf("status = %q, want warning from its own quiet sources", merged.Status) + } +} + func TestResourceRegistryUsesConfiguredProxmoxStaleThresholds(t *testing.T) { seen := time.Now().UTC().Add(-90 * time.Second).Truncate(time.Millisecond) snapshot := models.StateSnapshot{ From ade799307a54de6ea12325251cc2f99347d33385 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 13:27:20 +0100 Subject: [PATCH 2/9] Rank a confirmed availability outage first in the summary check A resource with several attached availability checks exposes one compatibility summary, Resource.Availability, and health reads only that. The summary ranked every observed failure equally and broke ties by target id, so a check that had failed once below its threshold could hide a later-sorting check with a confirmed outage: the outage check's own row carried availability_unreachable while the machine it monitors read healthy. Rank a confirmed outage above any other failure and have health use the same per-check rule, so target health and row presentation follow a confirmed outage on any attached check. The rule mirrors the poller's incident gate: observed failures at the threshold, an unavailable aggregate when one is reported, and no indeterminate outcome. A probe agent that stops reporting keeps its old failure count but reads indeterminate with an unknown aggregate, so it confirms nothing, on its own row or on the target. Drawers already list every check from availabilityChecks; the summary keeps its worst-check meaning. --- .../internal/subsystems/unified-resources.md | 14 ++ internal/unifiedresources/availability.go | 36 ++++- .../availability_link_test.go | 127 ++++++++++++++++++ internal/unifiedresources/health.go | 11 +- internal/unifiedresources/health_test.go | 2 +- 5 files changed, 178 insertions(+), 12 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index 53e49325b6..5a4ebb001d 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -3443,6 +3443,20 @@ It owns probe status, incidents, history, evidence, and the outgoing `checks` relationship. The matched resource carries every correlated check in the canonical `availabilityChecks` facet, keyed by saved target id, while `availability` remains an additive singular compatibility summary selected from that set. +The summary is the worst check: a confirmed outage first, then any other +observed failure, then an unchecked, ambiguous or unresolved check, then a +passing one, ties going to the first saved target id. A confirmed outage is an +enabled check whose observed failures reach its failure threshold, with an +unavailable aggregate when one is reported and an outcome that is not +indeterminate. That is the gate the availability poller applies before it +raises `availability_unreachable`, so a probe agent that stops reporting keeps +its old failure count but confirms nothing. `availabilityOutageConfirmed` in +`internal/unifiedresources/availability.go` holds that rule for both the +ranking and `EvaluateResourceHealth`, which reads the summary. A confirmed +outage on any attached check therefore makes the resource's health critical +even when a check that sorts earlier has failed below its threshold, with the +`availability_failed` reason unless a critical alert explains the verdict +first. Adding a second explicit or unambiguously correlated check must retain both source-owned endpoint rows, project both facets onto the same resource, and emit one `checks` relationship per target from the check to that resource. diff --git a/internal/unifiedresources/availability.go b/internal/unifiedresources/availability.go index 35620d04da..87c5d9ac21 100644 --- a/internal/unifiedresources/availability.go +++ b/internal/unifiedresources/availability.go @@ -99,6 +99,10 @@ func availabilityCheckKey(check AvailabilityData) string { return fmt.Sprintf("endpoint:%s:%s:%d:%s", protocol, address, check.Port, path) } +// primaryAvailabilityCheck selects the singular compatibility summary: the +// worst check, ties going to the first by key. Health and row presentation +// read only the summary, so a confirmed outage on any check must outrank a +// failure that has not yet reached its threshold. func primaryAvailabilityCheck(checks []AvailabilityData) *AvailabilityData { if len(checks) == 0 { return nil @@ -113,13 +117,39 @@ func primaryAvailabilityCheck(checks []AvailabilityData) *AvailabilityData { } func availabilityCheckPriority(check AvailabilityData) int { - if check.LastChecked != nil && !check.Available { + if availabilityOutageConfirmed(check) { return 0 } + if check.LastChecked != nil && !check.Available { + return 1 + } if check.LastChecked == nil || check.CorrelationState == AvailabilityCorrelationAmbiguous || check.CorrelationState == AvailabilityCorrelationUnresolved { - return 1 + return 2 } - return 2 + return 3 +} + +// availabilityOutageConfirmed reports whether an enabled check has failed at +// least its failure threshold in a row, by the same gate the availability +// poller uses before raising availability_unreachable: an observed failure +// whose aggregate, when present, is unavailable. A probe agent that stops +// reporting keeps its failure count but reads indeterminate with an unknown +// aggregate, and confirms nothing. +func availabilityOutageConfirmed(check AvailabilityData) bool { + if !check.Enabled || check.Available || check.LastChecked == nil { + return false + } + if state := strings.TrimSpace(check.AggregateState); state != "" && !strings.EqualFold(state, "unavailable") { + return false + } + if strings.EqualFold(strings.TrimSpace(check.ProbeOutcome), "indeterminate") { + return false + } + threshold := check.FailureThreshold + if threshold <= 0 { + threshold = 1 + } + return check.ConsecutiveFailures >= threshold } diff --git a/internal/unifiedresources/availability_link_test.go b/internal/unifiedresources/availability_link_test.go index 1daaa01432..fcb21d0289 100644 --- a/internal/unifiedresources/availability_link_test.go +++ b/internal/unifiedresources/availability_link_test.go @@ -403,6 +403,133 @@ func TestAvailabilityKeepsEveryConfiguredCheckWithMultipleServicesOnOneHost(t *t } } +func TestAvailabilitySummaryPrefersConfirmedOutageOverEarlierUnconfirmedFailure(t *testing.T) { + rr := NewRegistry(nil) + hostID := ingestAgentFixture(t, rr, "host-1", "machine-1") + now := time.Now().UTC() + check := func(targetID, address string, facet AvailabilityData) IngestRecord { + checkedAt := now + facet.LinkedResourceID = hostID + facet.Address = address + facet.Protocol = "https" + facet.Enabled = true + facet.LastChecked = &checkedAt + facet.FailureThreshold = 3 + return availabilityProbeRecord(targetID, address, &facet) + } + + // probe-a sorts first by target id and has failed once, below its + // threshold. probe-b is past its threshold and owns the outage incident. + unconfirmed := check("probe-a", "203.0.113.10", AvailabilityData{ + AggregateState: "unavailable", + ConsecutiveFailures: 1, + ApplicationFailureCode: "status_mismatch", + }) + unconfirmed.Resource.Status = StatusWarning + confirmed := check("probe-b", "203.0.113.11", AvailabilityData{ + AggregateState: "unavailable", + ConsecutiveFailures: 3, + }) + confirmed.Resource.Status = StatusOffline + confirmed.Resource.Incidents = []ResourceIncident{{ + Provider: string(SourceAvailability), + NativeID: "probe-b", + Code: "availability_unreachable", + }} + rr.IngestRecords(SourceAvailability, []IngestRecord{unconfirmed, confirmed}) + + host, ok := rr.Get(hostID) + if !ok || host == nil { + t.Fatalf("host %q missing after ingest", hostID) + } + if len(AvailabilityChecksForResource(*host)) != 2 { + t.Fatalf("availability checks = %+v, want both attached checks", host.AvailabilityChecks) + } + if host.Availability == nil || host.Availability.TargetID != "probe-b" { + t.Fatalf("availability summary = %+v, want the confirmed outage on probe-b", host.Availability) + } + health := EvaluateResourceHealth(*host, nil, now) + if health.Verdict != HealthCritical || len(health.Reasons) == 0 || + health.Reasons[0].Code != "availability_failed" || health.Reasons[0].Detail != "" { + t.Fatalf("host health = %+v, want availability_failed from probe-b", health) + } + + // Once probe-b recovers, the unconfirmed failure is the worst check left: + // it is the summary again, and it does not make the host an outage. + rr.IngestRecords(SourceAvailability, []IngestRecord{ + check("probe-b", "203.0.113.11", AvailabilityData{ + AggregateState: "healthy", + Available: true, + }), + }) + host, _ = rr.Get(hostID) + if host.Availability == nil || host.Availability.TargetID != "probe-a" { + t.Fatalf("availability summary = %+v, want the remaining failure on probe-a", host.Availability) + } + health = EvaluateResourceHealth(*host, nil, now) + for _, reason := range health.Reasons { + if reason.Code == "availability_failed" { + t.Fatalf("host health = %+v, want no outage below the failure threshold", health) + } + } +} + +func TestAvailabilitySummaryDoesNotConfirmOutageFromSilentProbeAgent(t *testing.T) { + rr := NewRegistry(nil) + hostID := ingestAgentFixture(t, rr, "host-1", "machine-1") + now := time.Now().UTC() + checkedAt := now + unconfirmed := availabilityProbeRecord("probe-a", "203.0.113.10", &AvailabilityData{ + LinkedResourceID: hostID, + Address: "203.0.113.10", + Protocol: "https", + Enabled: true, + AggregateState: "unavailable", + ProbeOutcome: "unreachable", + LastChecked: &checkedAt, + ConsecutiveFailures: 1, + FailureThreshold: 3, + }) + unconfirmed.Resource.Status = StatusWarning + + // The poller's read of a single-location probe whose agent stopped + // reporting: the old failure count survives, but the outcome is + // indeterminate, the aggregate unknown, and no incident is raised. + lastReport := now.Add(-time.Hour) + silent := availabilityProbeRecord("probe-b", "203.0.113.11", &AvailabilityData{ + LinkedResourceID: hostID, + Address: "203.0.113.11", + Protocol: "https", + ProbeAgentID: "agent-probe-1", + Enabled: true, + AggregateState: "unknown", + ProbeOutcome: "indeterminate", + ExpectedLocations: 1, + LastChecked: &lastReport, + ConsecutiveFailures: 5, + FailureThreshold: 3, + LastError: "no recent report from probe agent", + }) + silent.Resource.Status = StatusWarning + rr.IngestRecords(SourceAvailability, []IngestRecord{unconfirmed, silent}) + + host, ok := rr.Get(hostID) + if !ok || host == nil { + t.Fatalf("host %q missing after ingest", hostID) + } + if host.Availability == nil || host.Availability.TargetID != "probe-a" { + t.Fatalf("availability summary = %+v, want probe-a: a silent probe confirms no outage", host.Availability) + } + for _, resource := range []Resource{*host, availabilityEndpointByTarget(t, rr, "probe-b")} { + health := EvaluateResourceHealth(resource, nil, now) + for _, reason := range health.Reasons { + if reason.Code == "availability_failed" { + t.Fatalf("%s health = %+v, want no outage from a silent probe agent", resource.ID, health) + } + } + } +} + func TestAvailabilityEditReplacesEndpointAndMovesProjection(t *testing.T) { rr := NewRegistry(nil) hostA := ingestAgentFixture(t, rr, "host-a", "machine-a") diff --git a/internal/unifiedresources/health.go b/internal/unifiedresources/health.go index fabf9c7bd0..ef74dc69ea 100644 --- a/internal/unifiedresources/health.go +++ b/internal/unifiedresources/health.go @@ -187,15 +187,10 @@ func healthAlertSeverity(level string) int { } } +// availabilityFailureConfirmed reads the compatibility summary, which ranks a +// confirmed outage on any attached check first. func availabilityFailureConfirmed(resource Resource) bool { - if resource.Availability == nil || !resource.Availability.Enabled || resource.Availability.Available { - return false - } - threshold := resource.Availability.FailureThreshold - if threshold <= 0 { - threshold = 1 - } - return resource.Availability.ConsecutiveFailures >= threshold + return resource.Availability != nil && availabilityOutageConfirmed(*resource.Availability) } func availabilityFailureDetail(resource Resource) string { diff --git a/internal/unifiedresources/health_test.go b/internal/unifiedresources/health_test.go index c51de693e4..696733d0cb 100644 --- a/internal/unifiedresources/health_test.go +++ b/internal/unifiedresources/health_test.go @@ -31,7 +31,7 @@ func TestEvaluateResourceHealthPrecedenceAndVerdicts(t *testing.T) { }, { name: "confirmed availability failure is critical", - resource: Resource{ID: "check-1", Type: ResourceTypeNetworkEndpoint, Status: StatusOnline, Availability: &AvailabilityData{Enabled: true, Available: false, ConsecutiveFailures: 3, FailureThreshold: 3}}, + resource: Resource{ID: "check-1", Type: ResourceTypeNetworkEndpoint, Status: StatusOnline, Availability: &AvailabilityData{Enabled: true, Available: false, LastChecked: &now, ConsecutiveFailures: 3, FailureThreshold: 3}}, want: HealthCritical, code: "availability_failed", }, { From 27854a6da54e57b097f2fd0df7492deb397d5215 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 13:46:17 +0100 Subject: [PATCH 3/9] Stop websocket upgrade headers exempting demo-mode writes DemoModeMiddleware admitted any request carrying "Upgrade: websocket", whatever its method or path, so a POST/PUT/PATCH/DELETE with that header reached its handler on a read-only demo instance. The public demo signs visitors in as the configured admin user, which RequireAdmin accepts and which skips token scope checks, so the middleware was the only write guard: adding two headers let a demo session switch the instance out of mock mode, change system settings or create organisations. The exemption never served a real handshake. RFC 6455 handshakes are GET requests, gorilla's Upgrader refuses any other method, and GET already passes the read-only branch above it, so /ws and /api/agent/ws keep connecting. Removing the branch leaves the method alone deciding. TestContract_DemoModeUpgradeHeadersDoNotExemptWrites signs in as the demo admin through the full router, proves the session passes RequireAdmin, gets 403 for a mock-mode POST with upgrade headers and still opens /ws. On the old middleware the same POST returned 200 and turned mock mode off. --- .../v6/internal/subsystems/agent-lifecycle.md | 9 ++ .../v6/internal/subsystems/api-contracts.md | 11 ++ .../internal/subsystems/storage-recovery.md | 11 ++ internal/api/contract_test.go | 114 ++++++++++++++++++ internal/api/demo_middleware.go | 11 +- internal/api/demo_middleware_test.go | 83 ++++++++++++- 6 files changed, 230 insertions(+), 9 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index 21f4060e21..57249df605 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -8880,3 +8880,12 @@ agent from its last report instead of from the PVE poll, and mark it stale at their own heartbeat cutoff (`fleethealth.AgentStaleThreshold`, which is separate from the monitoring reporting lease). Agent registration, enrolment, install, update, removal and report identity are unchanged. + +### Demo write guard ignores websocket upgrade headers + +`internal/api/demo_middleware.go` changed only so a `POST`, `PUT`, `PATCH` or +`DELETE` carrying `Upgrade: websocket` is judged by the demo read-only guard +like any other write instead of always reaching its handler. Agent command +websocket handshakes to `/api/agent/ws` are HTTP/1.1 `GET` requests, the only +transport its gorilla upgrader serves, and still connect on a demo instance. +No agent registration, enrolment, install, update or removal path changed. diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index 978b841439..9daacf4b40 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -7645,6 +7645,17 @@ read-only demo account. That same hidden read-side boundary includes `GET` and `HEAD` reads for `/api/admin/users` and manual discovery at `/api/discover`; public demo mode may block writes generically, but it must not reveal that admin-user inventory or manual-discovery read routes exist. +That generic write block keys on the request method alone. The demo +middleware admits `GET`, `HEAD` and `OPTIONS` plus the login, logout, AI +execute and OIDC login/callback allowlist; HTTP/1.1 websocket handshakes for +`/ws` and `/api/agent/ws` are `GET` requests and connect through that read +branch, so an `Upgrade: websocket` or `Connection: Upgrade` header never +carries a `POST`, `PUT`, `PATCH` or `DELETE` past the guard. A demo deployment that signs +visitors in as the configured admin, as the public demo does, has only this +guard between that session and admin writes such as `/api/system/mock-mode`. +`TestContract_DemoModeUpgradeHeadersDoNotExemptWrites` in +`internal/api/contract_test.go` signs in as that admin through the full router +and pins both halves. That shared infrastructure install boundary now also preserves copied shell command payload continuity: any privilege-escalation wrapper applied at `frontend-modern/src/components/Settings/InfrastructureInstallerSection.tsx` diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index 66185fb88d..b38a046d36 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -6860,3 +6860,14 @@ migration or recovery authority is added or moved. The performance report and reporting runtime snapshot handlers apply the same test before tabulating a disk temperature. That changes only which held value a report shows, not any storage or recovery path. + +### Demo write guard ignores websocket upgrade headers + +`internal/api/demo_middleware.go` no longer exempts every request carrying +`Upgrade: websocket` from the demo read-only guard. On a `DEMO_MODE` instance a +`POST`, `PUT`, `PATCH` or `DELETE` with websocket handshake headers is now +judged like any other write: outside the login, logout, AI execute and OIDC +allowlist it gets the generic demo `403`, or `404` on a hidden route, and that +includes config import at `/api/config/import`. HTTP/1.1 websocket handshakes +are `GET` requests and still connect. No storage, retention, backup, migration +or recovery path is added or moved. diff --git a/internal/api/contract_test.go b/internal/api/contract_test.go index 1adcbc7d63..953ed0fba2 100644 --- a/internal/api/contract_test.go +++ b/internal/api/contract_test.go @@ -60,6 +60,7 @@ import ( "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" "github.com/rcourtman/pulse-go-rewrite/internal/updates" "github.com/rcourtman/pulse-go-rewrite/internal/vmware" + pulsews "github.com/rcourtman/pulse-go-rewrite/internal/websocket" agentshost "github.com/rcourtman/pulse-go-rewrite/pkg/agents/host" "github.com/rcourtman/pulse-go-rewrite/pkg/aicontracts" "github.com/rcourtman/pulse-go-rewrite/pkg/audit" @@ -9261,6 +9262,119 @@ func TestContract_DemoModeCommercialSurfacePolicy(t *testing.T) { }) } +// The public demo signs every visitor in as the configured admin, so admin +// routes accept a demo session and the demo guard is the only thing between it +// and a write. Websocket handshake headers must not carry a write past that +// guard, while a genuine handshake from the same session must still connect. +func TestContract_DemoModeUpgradeHeadersDoNotExemptWrites(t *testing.T) { + setMockModeForTest(t, true) + + dataDir := t.TempDir() + hashedPass, err := authpkg.HashPassword("demo") + if err != nil { + t.Fatalf("hash password: %v", err) + } + cfg := &config.Config{ + DataPath: dataDir, + ConfigPath: dataDir, + DemoMode: true, + AuthUser: "demo", + AuthPass: hashedPass, + } + + monitor, err := monitoring.New(cfg) + if err != nil { + t.Fatalf("new monitor: %v", err) + } + t.Cleanup(func() { monitor.Stop() }) + if err := monitor.SetMockMode(true); err != nil { + t.Fatalf("set monitor mock mode: %v", err) + } + + hub := pulsews.NewHub(nil) + go hub.Run() + t.Cleanup(hub.Stop) + router := NewRouter(cfg, monitor, nil, hub, nil, "1.0.0") + cleanupTestRouter(t, router) + server := newIPv4HTTPServer(t, router.Handler()) + t.Cleanup(server.Close) + + loginResp, err := http.Post(server.URL+"/api/login", "application/json", strings.NewReader(`{"username":"demo","password":"demo"}`)) + if err != nil { + t.Fatalf("demo login: %v", err) + } + loginResp.Body.Close() + if loginResp.StatusCode != http.StatusOK { + t.Fatalf("demo login status = %d, want %d", loginResp.StatusCode, http.StatusOK) + } + var cookiePairs []string + csrfToken := "" + for _, cookie := range loginResp.Cookies() { + cookiePairs = append(cookiePairs, cookie.Name+"="+cookie.Value) + if cookie.Name == CookieNameCSRF { + csrfToken = cookie.Value + } + } + if csrfToken == "" { + t.Fatal("expected CSRF cookie after demo login") + } + sessionCookies := strings.Join(cookiePairs, "; ") + + send := func(method, path, body string, headers map[string]string) (int, string) { + t.Helper() + req, err := http.NewRequest(method, server.URL+path, strings.NewReader(body)) + if err != nil { + t.Fatalf("build %s %s: %v", method, path, err) + } + req.Header.Set("Cookie", sessionCookies) + for key, value := range headers { + req.Header.Set(key, value) + } + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("%s %s: %v", method, path, err) + } + defer resp.Body.Close() + var payload bytes.Buffer + _, _ = payload.ReadFrom(resp.Body) + return resp.StatusCode, payload.String() + } + + if status, body := send(http.MethodGet, "/api/system/mock-mode", "", nil); status != http.StatusOK { + t.Fatalf("demo session read of admin route status = %d, want %d: %s", status, http.StatusOK, body) + } + + status, body := send(http.MethodPost, "/api/system/mock-mode", `{"enabled":false}`, map[string]string{ + "Content-Type": "application/json", + "X-CSRF-Token": csrfToken, + "Connection": "Upgrade", + "Upgrade": "websocket", + "Sec-WebSocket-Version": "13", + "Sec-WebSocket-Key": "dGhlIHNhbXBsZSBub25jZQ==", + }) + if status != http.StatusForbidden || !strings.Contains(body, "Demo mode enabled") { + t.Fatalf("demo write with websocket upgrade headers status = %d, want %d demo block: %s", status, http.StatusForbidden, body) + } + if !mock.IsMockEnabled() { + t.Fatal("demo write with websocket upgrade headers switched mock mode off") + } + + wsHeaders := wsHeadersForHTTP(t, server.URL) + wsHeaders.Set("Cookie", sessionCookies) + conn, resp, err := websocket.DefaultDialer.Dial(wsURLForHTTP(server.URL)+"/ws", wsHeaders) + if err != nil { + status := 0 + if resp != nil { + status = resp.StatusCode + } + t.Fatalf("demo session websocket handshake failed: %v (status %d)", err, status) + } + defer conn.Close() + if resp.StatusCode != http.StatusSwitchingProtocols { + t.Fatalf("demo session websocket handshake status = %d, want %d", resp.StatusCode, http.StatusSwitchingProtocols) + } +} + func TestContract_ReleaseDemoFixtureRuntimeGuardrailsRemainCanonical(t *testing.T) { requiredSnippets := map[string][]string{ "licensing_handlers.go": { diff --git a/internal/api/demo_middleware.go b/internal/api/demo_middleware.go index 7b55a8bf7c..3584529b27 100644 --- a/internal/api/demo_middleware.go +++ b/internal/api/demo_middleware.go @@ -33,18 +33,15 @@ func DemoModeMiddleware(cfg *config.Config, next http.Handler) http.Handler { } } - // Allow GET and HEAD requests (read-only) + // Allow GET and HEAD requests (read-only). WebSocket handshakes are GET + // requests (RFC 6455 section 4.1), so /ws and /api/agent/ws connect + // through this branch. An Upgrade header never exempts another method: + // a POST carrying "Upgrade: websocket" is still a write. if r.Method == http.MethodGet || r.Method == http.MethodHead || r.Method == http.MethodOptions { next.ServeHTTP(w, r) return } - // Allow WebSocket upgrades - if strings.ToLower(r.Header.Get("Upgrade")) == "websocket" { - next.ServeHTTP(w, r) - return - } - // Allow authentication endpoints (login is read-only - verifies credentials) authPaths := []string{ "/api/login", diff --git a/internal/api/demo_middleware_test.go b/internal/api/demo_middleware_test.go index 3ed38b032f..b4afd8aa97 100644 --- a/internal/api/demo_middleware_test.go +++ b/internal/api/demo_middleware_test.go @@ -4,8 +4,11 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strings" "testing" + "time" + "github.com/gorilla/websocket" "github.com/rcourtman/pulse-go-rewrite/internal/config" ) @@ -44,8 +47,8 @@ func TestDemoModeMiddleware(t *testing.T) { {"demo on websocket GET", true, http.MethodGet, "/api/ws", "websocket", true, http.StatusOK, true}, {"demo on websocket case insensitive", true, http.MethodGet, "/api/ws", "WebSocket", true, http.StatusOK, true}, {"demo on websocket uppercase", true, http.MethodGet, "/api/ws", "WEBSOCKET", true, http.StatusOK, true}, - // WebSocket upgrade with POST method (tests websocket branch after GET/HEAD/OPTIONS check) - {"demo on websocket POST", true, http.MethodPost, "/api/ws", "websocket", true, http.StatusOK, true}, + // An Upgrade header does not turn a write into a websocket handshake + {"demo on websocket POST", true, http.MethodPost, "/api/ws", "websocket", false, http.StatusForbidden, true}, // Demo mode enabled - auth endpoints allowed (POST) {"demo on login", true, http.MethodPost, "/api/login", "", true, http.StatusOK, true}, @@ -172,3 +175,79 @@ func TestDemoModeMiddleware_BlockedResponse(t *testing.T) { t.Error("message should not be empty") } } + +// A write that carries websocket handshake headers is still a write. The demo +// guard used to wave through any request with "Upgrade: websocket", so a demo +// session could switch mock mode off or change settings by adding two headers. +func TestDemoModeMiddlewareUpgradeHeadersDoNotExemptWrites(t *testing.T) { + handler := DemoModeMiddleware(&config.Config{DemoMode: true}, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Errorf("write with websocket upgrade headers reached the handler: %s %s", r.Method, r.URL.Path) + })) + + methods := []string{http.MethodPost, http.MethodPut, http.MethodPatch, http.MethodDelete} + paths := []string{"/api/system/mock-mode", "/api/system/settings/update", "/api/orgs", "/ws", "/api/agent/ws"} + for _, method := range methods { + for _, path := range paths { + req := httptest.NewRequest(method, path, strings.NewReader(`{"enabled":false}`)) + req.Header.Set("Connection", "Upgrade") + req.Header.Set("Upgrade", "websocket") + req.Header.Set("Sec-WebSocket-Version", "13") + req.Header.Set("Sec-WebSocket-Key", "dGhlIHNhbXBsZSBub25jZQ==") + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + if rec.Code != http.StatusForbidden { + t.Errorf("%s %s with websocket upgrade headers: status = %d, want %d", method, path, rec.Code, http.StatusForbidden) + } + } + } +} + +func TestDemoModeMiddlewareAllowsWebSocketHandshake(t *testing.T) { + upgrader := websocket.Upgrader{CheckOrigin: func(*http.Request) bool { return true }} + echo := func(w http.ResponseWriter, r *http.Request) { + conn, err := upgrader.Upgrade(w, r, nil) + if err != nil { + return + } + defer conn.Close() + messageType, payload, err := conn.ReadMessage() + if err != nil { + return + } + _ = conn.WriteMessage(messageType, payload) + } + mux := http.NewServeMux() + mux.HandleFunc("/ws", echo) + mux.HandleFunc("/api/agent/ws", echo) + + server := newIPv4HTTPServer(t, DemoModeMiddleware(&config.Config{DemoMode: true}, mux)) + defer server.Close() + + for _, path := range []string{"/ws", "/api/agent/ws"} { + conn, resp, err := websocket.DefaultDialer.Dial(wsURLForHTTP(server.URL)+path, nil) + if err != nil { + status := 0 + if resp != nil { + status = resp.StatusCode + } + t.Fatalf("websocket handshake to %s failed in demo mode: %v (status %d)", path, err, status) + } + if resp.StatusCode != http.StatusSwitchingProtocols { + conn.Close() + t.Fatalf("websocket handshake to %s: status = %d, want %d", path, resp.StatusCode, http.StatusSwitchingProtocols) + } + if err := conn.SetReadDeadline(time.Now().Add(5 * time.Second)); err != nil { + conn.Close() + t.Fatalf("set read deadline for %s: %v", path, err) + } + if err := conn.WriteMessage(websocket.TextMessage, []byte("ping")); err != nil { + conn.Close() + t.Fatalf("write to %s: %v", path, err) + } + _, payload, err := conn.ReadMessage() + conn.Close() + if err != nil || string(payload) != "ping" { + t.Fatalf("echo from %s = %q, %v; want %q", path, payload, err, "ping") + } + } +} From 27e440339b7a02bed248628d2ad85aff6587f53d Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 13:45:11 +0100 Subject: [PATCH 4/9] Switch every running tenant monitor when mock mode flips Mock mode is one process-wide flag, but SetMockMode ended the epoch, cleared the alerts, forgot the fixture agents and reset the state of only the monitor it was called on. The server runs one monitor per organization, each with its own alert manager and its own mock alert pass. A switch made through one of them (the mock-mode API uses the requesting tenant's monitor, the demo-fixture licence sync the default one) left every other running monitor holding what it raised in the mode the process had left: fixture alerts and fixture-agent node links in live mode, live alerts in mock mode, until the 24-hour stale sweep. Monitors now join a process-wide set of running monitors when Start runs and leave it when Start returns, under the same mutex that already serialized switches. SetMockMode flips the flag once, then ends the epoch of the monitor it was called on and of every running monitor: it waits for their admitted calls, clears their alerts, forgets their fixture agents and resets their state. Only then does it restart each running monitor's sampler, polls and discovery. Start picks the mock metrics sampler or discovery while it holds that mutex, so no switch can land between reading the mode and starting the runtime for it, and a switch leaves the runtime of a monitor that has not joined yet to its own Start. A monitor records the mode and the switch count it was built under. If it missed switches between New and Start, it leaves what it holds before it joins, dropping live alerts New restored or fixture alerts a read path raised meanwhile, even when a round trip brought the flag back. A call for the mode the flag already holds still moves a reached monitor whose alerts belong to the other mode, and leaves the rest alone. Release builds reach this only on an instance that runs DemoMode with the demo_fixtures entitlement, since release ValidateEnablement refuses mock fixtures otherwise, and that has more than one organization's monitor running. The licence sync only switches the default organization. So the fix stays inside the monitoring package. The callers are unchanged and no API surface moves. New tests start the default and one other tenant monitor through MultiTenantMonitor, as the server does, and switch through one of them. Against the per-monitor switch, the default monitor kept its fixture alerts (69 to 104 across runs) and all 69 fixture agents after mock mode was left through the other tenant. That tenant kept its live alert after mock mode was entered through the default monitor, and its fixture node stayed suppressed by a fixture agent's leftover node link. Four more tests fail if Start joins without leaving the mode its monitor was built in, if Start compares only the mode after a round trip, if a call for the flag's current mode returns without moving such a monitor, or if a switch starts the runtime of a monitor that has not joined. --- .../v6/internal/subsystems/agent-lifecycle.md | 9 + .../v6/internal/subsystems/monitoring.md | 52 +++- .../monitoring/canonical_guardrails_test.go | 23 +- internal/monitoring/mock_mode_switch.go | 75 ++++++ internal/monitoring/monitor.go | 203 ++++++++------ ...nitor_host_agent_removal_lifecycle_test.go | 43 +++ .../monitoring/monitor_mock_alerts_test.go | 249 ++++++++++++++++++ 7 files changed, 559 insertions(+), 95 deletions(-) create mode 100644 internal/monitoring/mock_mode_switch.go diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index e7b4f59ab3..7582e7db33 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -754,6 +754,15 @@ not fenced: a live report already past its mock-mode check when mock mode is switched on can still be evaluated after the clear, and its alerts can persist until mock mode is left. +Every tenant monitor registers its own fixture agents, and their node links, +in its own alert manager, so the toggle forgets the fixture agents of every +running monitor, not only those of the monitor it was called through. A link +left behind would keep owning its fixture node's usage alerts after mock mode +ends. `TestLeavingMockModeReleasesFixtureAgentNodeLinksOnEveryRunningMonitor` +in `internal/monitoring/monitor_host_agent_removal_lifecycle_test.go` switches +through the default monitor and fails if the other tenant's node stays +suppressed. + Physical-disk evidence collected by a host agent must survive projection back into monitoring's models. Absent evidence has to carry its declared sentinel rather than a zero value that reads as a real measurement: an absent diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 60ff94e27d..7e54def128 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -1855,11 +1855,33 @@ a live report or poll already past its mock-mode check when mock mode is switched on can still evaluate after that clear, and its alerts can persist until mock mode is left. -The fence and the clear are per monitor while the mode is process-wide. A -tenant monitor other than the one `SetMockMode` was called on neither drains -nor clears, so fixture alerts it raised outlive the switch as they did before -the fence; switching mock mode with several tenant monitors running is not -covered. +The mode is process-wide while each monitor keeps its own fence, alert manager, +fixture agents and state, and the server runs one monitor per organization, so +`SetMockMode` switches every running monitor, whichever tenant's monitor it is +called on (the mock-mode API calls the requesting tenant's, the demo-fixture +licence sync the default organization's). `Start` joins a process-wide set of +running monitors under `mockModeSwitchMu`, and leaves it when its loop returns. +A switch flips the mode once, then ends each monitor's epoch, waits for its +admitted calls, clears its alerts, forgets its fixture agents when leaving mock +mode and resets its state, and only then restarts the mode-dependent runtime of +each monitor in the set. `Start` sets its runtime context before it joins and +chooses the mock metrics sampler or discovery while it joins, under the same +lock, so no switch lands between reading the mode and starting the runtime for +it, and a switch that reaches a monitor not yet joined leaves the runtime to +its `Start`. A monitor that missed switches between `New` and `Start` first +leaves what it holds: `New` records the mode and the number of switches made so +far, and `Start` compares both, so even a round trip that brought the flag back +drops the fixture alerts a read path raised meanwhile, as well as the live +alerts `New` restored outside mock mode. A monitor that is not running is +switched only when the switch is made through it; a call for the mode the flag +already holds still moves any reached monitor whose alerts belong to the other +mode, and leaves the rest alone. + +Release builds reach several tenant monitors in mock mode only on an instance +running `DemoMode` with the `demo_fixtures` entitlement (release +`ValidateEnablement` refuses mock fixtures otherwise) and with more than one +organization's monitor running; the licence sync switches only for the +default organization. The proofs park a real evaluation at a fixed point, switch modes and release it. `TestLeavingMockModeRefusesTheRestOfAnInFlightPass` holds a mock pass at its @@ -1889,7 +1911,25 @@ previous-state carry returns fixture nodes or guests. All eight are in `TestStoreRefreshTakesItsMockModeScopeBeforeReadingState` in `internal/monitoring/canonical_guardrails_test.go` rejects a call that reads state in an argument ahead of `m.mockModeFence.begin()` and pins the -end-epoch-then-clear order in `SetMockMode`. +end-epoch-then-clear order in `endMockModeEpoch`, which `SetMockMode` runs for +every monitor it reaches. + +`TestLeavingMockModeClearsEveryRunningTenantMonitor` and +`TestEnteringMockModeClearsEveryRunningTenantMonitor` start the default and one +other tenant monitor through `MultiTenantMonitor`, wait until both have joined, +raise alerts in both, switch through one of them and check both. Against a +switch that reached only its own monitor, the default monitor kept its fixture +alerts (69 to 104 across runs) and all 69 fixture agents after leaving mock +mode through the other tenant, and that tenant kept its live alert after mock +mode was entered through the default monitor. `TestMonitorStartedAfterAMockModeSwitchLeavesTheModeItWasBuiltIn` +fails if `Start` joins without leaving the mode its monitor was built in, and +`TestSetMockModeAlignsAMonitorWhoseModeTheFlagAlreadyHolds` fails if a call for +the flag's current mode returns without moving such a monitor, and +`TestMockModeSwitchLeavesAnUnjoinedMonitorsRuntimeToItsStart` fails if a switch +starts the runtime of a monitor whose `Start` has not joined, and +`TestMonitorStartedAfterAMockModeRoundTripDropsWhatItRaisedMeanwhile` fails if +`Start` compares only the mode. All six are in +`internal/monitoring/monitor_mock_alerts_test.go`. The disk evaluation runs on the mock alert tick, not the poller's disk interval (five minutes by default, `PhysicalDiskPollingMinutes` per instance). diff --git a/internal/monitoring/canonical_guardrails_test.go b/internal/monitoring/canonical_guardrails_test.go index fd27c2215b..8a13f8b631 100644 --- a/internal/monitoring/canonical_guardrails_test.go +++ b/internal/monitoring/canonical_guardrails_test.go @@ -543,8 +543,12 @@ func TestStoreRefreshTakesItsMockModeScopeBeforeReadingState(t *testing.T) { if err != nil { t.Fatalf("failed to read monitor.go: %v", err) } - if got := strings.Count(string(data), "m.mockModeFence.advance()\n\t\tm.alertManager.ClearActiveAlerts()"); got != 2 { - t.Fatalf("SetMockMode must end the mock-mode epoch immediately before clearing alerts in both directions, found %d", got) + source := string(data) + if got := strings.Count(source, "m.mockModeFence.advance()\n\tm.alertManager.ClearActiveAlerts()"); got != 1 { + t.Fatalf("endMockModeEpoch must end the mock-mode epoch immediately before clearing alerts, found %d", got) + } + if !strings.Contains(source, "for _, monitor := range monitors {\n\t\tmonitor.endMockModeEpoch(enable)\n\t}") { + t.Fatal("SetMockMode must end the epoch of every monitor the switch reaches") } } @@ -1664,16 +1668,11 @@ func TestMonitorSetMockModeAuthorizesBeforeResettingRuntimeState(t *testing.T) { if err != nil { t.Fatalf("failed to read monitor.go: %v", err) } - source := string(data) - requiredSnippets := []string{ - "if err := mock.SetEnabled(true); err != nil {", - "if err := mock.SetEnabled(false); err != nil {", - "return err", - } - for _, snippet := range requiredSnippets { - if !strings.Contains(source, snippet) { - t.Fatalf("monitor.go must contain %q", snippet) - } + source := strings.Join(strings.Fields(string(data)), " ") + authorize := strings.Index(source, "if err := mock.SetEnabled(enable); err != nil { return err }") + reset := strings.Index(source, "monitor.endMockModeEpoch(enable)") + if authorize < 0 || reset < 0 || authorize > reset { + t.Fatal("SetMockMode must return the mock.SetEnabled error before it ends any monitor's epoch or resets its state") } } diff --git a/internal/monitoring/mock_mode_switch.go b/internal/monitoring/mock_mode_switch.go new file mode 100644 index 0000000000..4419f77b88 --- /dev/null +++ b/internal/monitoring/mock_mode_switch.go @@ -0,0 +1,75 @@ +package monitoring + +import ( + "sync" + "sync/atomic" + + "github.com/rcourtman/pulse-go-rewrite/internal/mock" +) + +// Mock mode is process-wide (mock.SetEnabled flips one package flag), while +// every Monitor keeps its own alert manager, fixture agents, state and +// mock-mode fence, and the server runs one Monitor per organization. A switch +// made through one monitor therefore has to reach every running monitor, or +// the others keep what they raised in the mode the process left: fixture +// alerts and fixture-agent node links in live mode, live alerts in mock mode, +// until the 24-hour stale sweep. +// +// mockModeSwitchMu serializes switches and guards the set of running monitors +// and each monitor's mockModeAligned and mockModeSwitchesSeen. A switch is +// several steps per monitor (end the epoch, clear, reset), so two interleaved +// switches could clear what the later one just admitted. Start joins the set +// under the same lock, so a monitor joins either before a switch, which then +// switches it with the others, or after it, and starts in the new mode. +// +// mockModeSwitchCount counts the flips SetMockMode made. A monitor that missed +// flips while it was not running can hold the alerts and state of a mode the +// process left even when a later flip brought the flag back to the mode it +// records (a read path evaluated fixture data before Start), so Start +// compares the count as well as the mode. +var ( + mockModeSwitchMu sync.Mutex + mockModeRunningMonitors = make(map[*Monitor]struct{}) + mockModeSwitchCount atomic.Uint64 +) + +// mockModeSwitchTargetsLocked returns the monitors a switch made through m +// reaches: m itself, which need not be running, and every running monitor. +func mockModeSwitchTargetsLocked(m *Monitor) []*Monitor { + targets := make([]*Monitor, 0, len(mockModeRunningMonitors)+1) + targets = append(targets, m) + for monitor := range mockModeRunningMonitors { + if monitor != m { + targets = append(targets, monitor) + } + } + return targets +} + +// joinMockModeSwitches adds m to the monitors every switch reaches, then runs +// start with the current mode while no switch can run, so the mode-dependent +// runtime start chooses (the mock metrics sampler or discovery) cannot be +// overtaken by a switch. It returns the function that removes m again. +// +// A switch can land between New and Start, while m is not yet running. New +// restores persisted alerts only outside mock mode, so m would then carry live +// alerts into mock mode, or fixture alerts that a read path raised before +// Start into live mode; m first leaves what it holds, as the switches it +// missed would have done. +func (m *Monitor) joinMockModeSwitches(start func(mockEnabled bool)) (leave func()) { + mockModeSwitchMu.Lock() + defer mockModeSwitchMu.Unlock() + + enabled := mock.IsMockEnabled() + if m.mockModeAligned != enabled || m.mockModeSwitchesSeen != mockModeSwitchCount.Load() { + m.endMockModeEpoch(enabled) + } + mockModeRunningMonitors[m] = struct{}{} + start(enabled) + + return func() { + mockModeSwitchMu.Lock() + defer mockModeSwitchMu.Unlock() + delete(mockModeRunningMonitors, m) + } +} diff --git a/internal/monitoring/monitor.go b/internal/monitoring/monitor.go index 69c5c33f37..80fc4aa163 100644 --- a/internal/monitoring/monitor.go +++ b/internal/monitoring/monitor.go @@ -13,6 +13,7 @@ import ( "os" "path/filepath" "reflect" + "slices" "sort" "strings" "sync" @@ -67,9 +68,9 @@ func keepRealPollingInMockMode() bool { // process that boots with mock mode already enabled never runs that path, so // alerts raised against real infrastructure are restored from disk and served // beside fixture data until something clears them. -func alertManagerRestoreOptions() []alerts.ManagerOption { +func alertManagerRestoreOptions(mockEnabled bool) []alerts.ManagerOption { options := []alerts.ManagerOption{alerts.WithDurableAlertStore()} - if mock.IsMockEnabled() { + if mockEnabled { options = append(options, alerts.WithoutPersistedAlertRestore()) } return options @@ -1283,6 +1284,8 @@ type Monitor struct { mockHostAgentsMu sync.Mutex mockHostAgents map[string]models.Host // Fixture agents evaluated by the last mock alert pass mockModeFence mockModeFence // Keeps mode-dependent alert evaluations inside the epoch they read in + mockModeAligned bool // Mock mode the alerts and state belong to; guarded by mockModeSwitchMu + mockModeSwitchesSeen uint64 // mockModeSwitchCount when they were last aligned; guarded by mockModeSwitchMu dockerChecker DockerChecker // Optional Docker checker for LXC containers dockerCheckerConfiguredAt time.Time // Last time the Docker checker was configured dockerCheckAllowedVMIDs map[int]struct{} // Optional VMID allowlist gating the LXC Docker socket probe; empty means all guests @@ -1771,6 +1774,12 @@ func New(cfg *config.Config) (*Monitor, error) { DataDir: cfg.DataPath, }) + // One read decides the alert restore and records the mode it belongs to, + // so Start can tell whether a mock-mode switch landed in between. The + // count is read first: a switch flips the flag before it counts, so one + // that overlaps these reads leaves a stale count, never a stale mode. + switchesSeen := mockModeSwitchCount.Load() + builtInMockMode := mock.IsMockEnabled() m := &Monitor{ config: cfg, state: models.NewState(), @@ -1804,7 +1813,9 @@ func New(cfg *config.Config) (*Monitor, error) { rateTracker: NewRateTracker(), metricsHistory: NewMetricsHistory(1000, 24*time.Hour), // Keep up to 1000 points (~8h @ 30s) metricsStore: metricsStore, // Persistent SQLite storage - alertManager: alerts.NewManagerWithDataDir(cfg.DataPath, alertManagerRestoreOptions()...), + alertManager: alerts.NewManagerWithDataDir(cfg.DataPath, alertManagerRestoreOptions(builtInMockMode)...), + mockModeAligned: builtInMockMode, + mockModeSwitchesSeen: switchesSeen, incidentStore: incidentStore, notificationMgr: notifications.NewNotificationManagerWithDeferredQueue(cfg.PublicURL, cfg.DataPath), deadMan: newDeadManRuntime(config.ResolveRuntimeDataDir(cfg.DataPath)), @@ -2086,33 +2097,13 @@ func (m *Monitor) Start(ctx context.Context, wsHub *websocket.Hub) { // PVE-proxied PBS backup points to prevent duplicate recovery entries. m.purgeStalePVEPBSBackupsBestEffort(ctx) - if mock.IsMockEnabled() { - m.startMockMetricsSampler(ctx) - } - - // Initialize and start discovery service if enabled - if mock.IsMockEnabled() { - log.Info().Msg("mock mode enabled - skipping discovery service") - m.discoveryService = nil - } else if m.config.DiscoveryEnabled { - discoverySubnet := m.config.DiscoverySubnet - if discoverySubnet == "" { - discoverySubnet = "auto" - } - cfgProvider := func() config.DiscoveryConfig { - return m.discoveryConfigSnapshot() - } - m.discoveryService = discovery.NewService(wsHub, 5*time.Minute, discoverySubnet, cfgProvider) - if m.discoveryService != nil { - m.discoveryService.Start(ctx) - log.Info().Msg("discovery service initialized and started") - } else { - log.Error().Msg("failed to initialize discovery service") - } - } else { - log.Info().Msg("discovery service disabled by configuration") - m.discoveryService = nil - } + // From here every mock-mode switch reaches this monitor. The sampler or + // discovery is chosen while no switch can run, so a switch cannot land + // between reading the mode and starting the runtime for it. + leaveMockModeSwitches := m.joinMockModeSwitches(func(mockEnabled bool) { + m.startModeRuntime(ctx, wsHub, mockEnabled) + }) + defer leaveMockModeSwitches() // Set up alert callbacks. Projection replay is deliberately absent here: // the canonical resource store is not attached yet, so a replay now could @@ -2251,6 +2242,38 @@ func (m *Monitor) Start(ctx context.Context, wsHub *websocket.Hub) { } } +// startModeRuntime starts the mock metrics sampler in mock mode, or the +// discovery service in live mode when it is enabled. +func (m *Monitor) startModeRuntime(ctx context.Context, wsHub *websocket.Hub, mockEnabled bool) { + if mockEnabled { + m.startMockMetricsSampler(ctx) + } + + // Initialize and start discovery service if enabled + if mockEnabled { + log.Info().Msg("mock mode enabled - skipping discovery service") + m.discoveryService = nil + } else if m.config.DiscoveryEnabled { + discoverySubnet := m.config.DiscoverySubnet + if discoverySubnet == "" { + discoverySubnet = "auto" + } + cfgProvider := func() config.DiscoveryConfig { + return m.discoveryConfigSnapshot() + } + m.discoveryService = discovery.NewService(wsHub, 5*time.Minute, discoverySubnet, cfgProvider) + if m.discoveryService != nil { + m.discoveryService.Start(ctx) + log.Info().Msg("discovery service initialized and started") + } else { + log.Error().Msg("failed to initialize discovery service") + } + } else { + log.Info().Msg("discovery service disabled by configuration") + m.discoveryService = nil + } +} + // wireExternalAlertCallbacks installs callbacks whose effects leave the alert // manager. It is safe to call again: these are single callback slots, so Start // replaces the constructor-time escalation closure with one carrying its hub. @@ -4577,65 +4600,93 @@ func (m *Monitor) broadcastEscalatedAlert(hub *websocket.Hub, alert *alerts.Aler hub.BroadcastAlertToTenant(m.GetOrgID(), alert) } -// mockModeSwitchMu serializes mock-mode switches. The mode is process-wide, -// and a switch is several steps (flip, end the epoch, clear, reset state), so -// two interleaved switches could clear what the later one just admitted. -var mockModeSwitchMu sync.Mutex - -// SetMockMode switches between mock data and real infrastructure data at runtime. -// It must not be called from inside an alert evaluation, whose completion it -// may wait for. +// SetMockMode switches between mock data and real infrastructure data at +// runtime. The mode is process-wide, so the switch reaches m and every running +// monitor (see mockModeSwitchMu), whichever tenant's monitor it was made +// through. It must not be called from inside an alert evaluation of any of +// them, whose completion it may wait for. func (m *Monitor) SetMockMode(enable bool) error { mockModeSwitchMu.Lock() defer mockModeSwitchMu.Unlock() - current := mock.IsMockEnabled() - if current == enable { - log.Info().Bool("mockMode", enable).Msg("mock mode already in desired state") - return nil + monitors := mockModeSwitchTargetsLocked(m) + flip := mock.IsMockEnabled() != enable + if !flip { + // The flag already holds the mode, but a monitor that was not running + // during an earlier switch can still hold the other mode's alerts + // and state; only those leave it. + monitors = slices.DeleteFunc(monitors, func(monitor *Monitor) bool { + return monitor.mockModeAligned == enable + }) + if len(monitors) == 0 { + log.Info().Bool("mockMode", enable).Msg("mock mode already in desired state") + return nil + } } - // Every evaluation of mode-dependent data that started before the flip - // must finish or be refused before the clear, or it reopens alerts for - // the side the monitor just left (see mockModeFence). + for _, monitor := range monitors { + monitor.stopMockMetricsSampler() + } + if flip { + if err := mock.SetEnabled(enable); err != nil { + return err + } + mockModeSwitchCount.Add(1) + } + for _, monitor := range monitors { + monitor.endMockModeEpoch(enable) + } + for _, monitor := range monitors { + _, running := mockModeRunningMonitors[monitor] + monitor.resumeAfterMockModeSwitch(enable, running) + } if enable { - m.stopMockMetricsSampler() - if err := mock.SetEnabled(true); err != nil { - return err - } - m.mockModeFence.advance() - m.alertManager.ClearActiveAlerts() - m.mu.Lock() - m.resetStateLocked() - m.metricsHistory.Reset() - m.mu.Unlock() - m.StopDiscoveryService() - m.mu.RLock() - ctx := m.runtimeCtx - m.mu.RUnlock() - if ctx != nil { - m.startMockMetricsSampler(ctx) - } - log.Info().Msg("switched monitor to mock mode") + log.Info().Int("monitors", len(monitors)).Msg("switched monitors to mock mode") } else { - m.stopMockMetricsSampler() - if err := mock.SetEnabled(false); err != nil { - return err - } - m.mockModeFence.advance() - m.alertManager.ClearActiveAlerts() - m.forgetMockHostAgents() - m.mu.Lock() - m.resetStateLocked() - m.metricsHistory.Reset() - m.mu.Unlock() - log.Info().Msg("switched monitor to real data mode") + log.Info().Int("monitors", len(monitors)).Msg("switched monitors to real data mode") } + return nil +} +// endMockModeEpoch leaves the mode the monitor's alerts and state belong to +// once the process flag already holds the new one. Every evaluation of +// mode-dependent data that started before the flip must finish or be refused +// before the clear, or it reopens alerts for the side the monitor just left +// (see mockModeFence). The caller holds mockModeSwitchMu. +func (m *Monitor) endMockModeEpoch(enable bool) { + m.mockModeFence.advance() + m.alertManager.ClearActiveAlerts() + if !enable { + m.forgetMockHostAgents() + } + m.mu.Lock() + m.resetStateLocked() + m.metricsHistory.Reset() + m.mu.Unlock() + if enable { + m.StopDiscoveryService() + } + m.mockModeAligned = enable + m.mockModeSwitchesSeen = mockModeSwitchCount.Load() +} + +// resumeAfterMockModeSwitch restarts the mode-dependent runtime of a monitor +// whose Start loop has joined the switches, once every monitor has ended its +// epoch. A monitor that has not joined yet starts that runtime itself when it +// does (Start sets its runtime context first), and one whose loop has stopped +// only broadcasts. +func (m *Monitor) resumeAfterMockModeSwitch(enable, running bool) { m.mu.RLock() ctx := m.runtimeCtx hub := m.wsHub m.mu.RUnlock() + if !running || (ctx != nil && ctx.Err() != nil) { + ctx = nil + } + + if enable && ctx != nil { + m.startMockMetricsSampler(ctx) + } if hub != nil { m.broadcastCurrentState(hub) @@ -4653,8 +4704,6 @@ func (m *Monitor) SetMockMode(enable bool) error { go m.StartDiscoveryService(ctx, hub, m.config.DiscoverySubnet) } } - - return nil } func (m *Monitor) resetStateLocked() { diff --git a/internal/monitoring/monitor_host_agent_removal_lifecycle_test.go b/internal/monitoring/monitor_host_agent_removal_lifecycle_test.go index dca2a68b1c..e9d81cf562 100644 --- a/internal/monitoring/monitor_host_agent_removal_lifecycle_test.go +++ b/internal/monitoring/monitor_host_agent_removal_lifecycle_test.go @@ -11,6 +11,7 @@ import ( "github.com/rcourtman/pulse-go-rewrite/internal/alerts" "github.com/rcourtman/pulse-go-rewrite/internal/alerts/eventlog" "github.com/rcourtman/pulse-go-rewrite/internal/config" + "github.com/rcourtman/pulse-go-rewrite/internal/mock" "github.com/rcourtman/pulse-go-rewrite/internal/models" "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" agentshost "github.com/rcourtman/pulse-go-rewrite/pkg/agents/host" @@ -857,3 +858,45 @@ func TestMockHostAgentLeavingFixtureUsesRemovalLifecycle(t *testing.T) { } t.Fatal("node pve9 raised no memory alert after its agent left the mock estate") } + +func TestLeavingMockModeReleasesFixtureAgentNodeLinksOnEveryRunningMonitor(t *testing.T) { + setMockSamplerTestEnv(t, time.Hour, 5*time.Minute) + pinDefaultMockEstate(t) + mustSetMockEnabled(t, true) + defaultMonitor, tenantMonitor := startTenantMonitors(t) + + // org-b's start pass registers the fixture agents in org-b's own alert + // manager, each owning the usage alerts of the fixture node it runs on. + var linked models.Node + waitForCondition(t, 30*time.Second, func() bool { + tenantMonitor.mockHostAgentsMu.Lock() + defer tenantMonitor.mockHostAgentsMu.Unlock() + for _, host := range tenantMonitor.mockHostAgents { + if host.Status != "online" || host.LinkedNodeID == "" { + continue + } + for _, node := range mock.CurrentFixtureGraph().State.Nodes { + if node.ID == host.LinkedNodeID { + linked = node + return true + } + } + } + return false + }, "org-b registered no online fixture agent linked to a fixture node") + + // The demo-fixture licence sync switches through the default monitor. + mustSetMonitorMockMode(t, defaultMonitor, false) + + // org-b's fixture agents go through the removal lifecycle too, so the + // node an agent covered owns its metric alerts again. + linked.Status = "online" + linked.Memory = models.Memory{Total: 64 << 30, Used: 60 << 30, Free: 4 << 30, Usage: 93.75} + tenantMonitor.alertManager.CheckNode(linked) + for _, alert := range tenantMonitor.alertManager.GetActiveAlerts() { + if alert.ResourceID == linked.ID && alert.Type == "memory" { + return + } + } + t.Fatalf("org-b's node %s raised no memory alert: its fixture agent's node link outlived a switch made through the default monitor", linked.ID) +} diff --git a/internal/monitoring/monitor_mock_alerts_test.go b/internal/monitoring/monitor_mock_alerts_test.go index 3c02565ece..ce0b725e54 100644 --- a/internal/monitoring/monitor_mock_alerts_test.go +++ b/internal/monitoring/monitor_mock_alerts_test.go @@ -15,6 +15,7 @@ import ( "github.com/rcourtman/pulse-go-rewrite/internal/alerts" "github.com/rcourtman/pulse-go-rewrite/internal/alerts/eventlog" + "github.com/rcourtman/pulse-go-rewrite/internal/config" "github.com/rcourtman/pulse-go-rewrite/internal/mock" "github.com/rcourtman/pulse-go-rewrite/internal/models" "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" @@ -809,6 +810,254 @@ func TestLeavingMockModeDiscardsAnInFlightConnectionCheck(t *testing.T) { } } +// startTenantMonitors starts the default organization's monitor and one +// other tenant's through MultiTenantMonitor, as the server does, each with its +// own alert manager, and waits until both monitoring loops run. An hour +// between ticks leaves each loop the pass it runs on start, so the test drives +// every later pass itself. +func startTenantMonitors(t *testing.T) (defaultMonitor, tenantMonitor *Monitor) { + t.Helper() + mtp, _ := newTestTenantPersistence(t) + mtm := NewMultiTenantMonitor(&config.Config{DataPath: t.TempDir(), PVEPollingInterval: time.Hour}, mtp, nil) + t.Cleanup(mtm.Stop) + mtm.SetMonitorInitializer(func(monitor *Monitor) { + cfg := monitor.alertManager.GetConfig() + cfg.Enabled = true + cfg.TimeThresholds = map[string]int{} + cfg.MetricTimeThresholds = nil + cfg.BackupDefaults.Enabled = false + cfg.AgentDefaults.Memory = &alerts.HysteresisThreshold{Trigger: 1, Clear: 0.5} + cfg.NodeDefaults.Memory = &alerts.HysteresisThreshold{Trigger: 1, Clear: 0.5} + monitor.alertManager.UpdateConfig(cfg) + }) + if err := mtp.SaveOrganization(&models.Organization{ID: "org-b", DisplayName: "Org B"}); err != nil { + t.Fatalf("SaveOrganization(org-b): %v", err) + } + monitors := make([]*Monitor, 0, 2) + for _, orgID := range []string{"default", "org-b"} { + monitor, err := mtm.GetMonitor(orgID) + if err != nil { + t.Fatalf("GetMonitor(%s): %v", orgID, err) + } + // Start sets its runtime context before it joins the monitors a + // switch reaches, so wait for the join itself. + waitForCondition(t, 30*time.Second, func() bool { + mockModeSwitchMu.Lock() + defer mockModeSwitchMu.Unlock() + _, running := mockModeRunningMonitors[monitor] + return running + }, orgID+" monitoring loop did not join mock-mode switches") + monitors = append(monitors, monitor) + } + return monitors[0], monitors[1] +} + +func mockHostAgentCount(monitor *Monitor) int { + monitor.mockHostAgentsMu.Lock() + defer monitor.mockHostAgentsMu.Unlock() + return len(monitor.mockHostAgents) +} + +func TestLeavingMockModeClearsEveryRunningTenantMonitor(t *testing.T) { + setMockSamplerTestEnv(t, time.Hour, 5*time.Minute) + pinDefaultMockEstate(t) + mustSetMockEnabled(t, true) + defaultMonitor, tenantMonitor := startTenantMonitors(t) + + // Each loop runs a mock pass on start and raises the fixture's alerts in + // its own alert manager. + monitors := map[string]*Monitor{"default": defaultMonitor, "org-b": tenantMonitor} + for name, monitor := range monitors { + waitForCondition(t, 30*time.Second, func() bool { + return len(monitor.alertManager.GetActiveAlerts()) > 0 && mockHostAgentCount(monitor) > 0 + }, name+" monitor raised no fixture alerts in mock mode") + } + + // The mock-mode API switches through the requesting tenant's monitor. + mustSetMonitorMockMode(t, tenantMonitor, false) + + for name, monitor := range monitors { + if active := monitor.alertManager.GetActiveAlerts(); len(active) != 0 { + t.Errorf("%s monitor kept %d fixture alerts after mock mode was left through org-b: %s", + name, len(active), describeAlerts(active)) + } + if count := mockHostAgentCount(monitor); count != 0 { + t.Errorf("%s monitor still tracks %d fixture agents in live mode", name, count) + } + } +} + +func TestEnteringMockModeClearsEveryRunningTenantMonitor(t *testing.T) { + setMockSamplerTestEnv(t, time.Hour, 5*time.Minute) + pinDefaultMockEstate(t) + defaultMonitor, tenantMonitor := startTenantMonitors(t) + + monitors := map[string]*Monitor{"default": defaultMonitor, "org-b": tenantMonitor} + for name, monitor := range monitors { + monitor.alertManager.CheckNode(models.Node{ + ID: name + "-live-node", + Name: "live-node", + Instance: "live", + Status: "online", + Memory: models.Memory{Total: 64 << 30, Used: 60 << 30, Free: 4 << 30, Usage: 93.75}, + }) + if len(monitor.alertManager.GetActiveAlerts()) == 0 { + t.Fatalf("live node raised no alert in %s", name) + } + } + + // The demo-fixture licence sync switches through the default monitor. + mustSetMonitorMockMode(t, defaultMonitor, true) + + for name, monitor := range monitors { + for _, alert := range monitor.alertManager.GetActiveAlerts() { + if alert.ResourceID == name+"-live-node" { + t.Errorf("%s kept its live %s alert after mock mode was entered through the default monitor", name, alert.Type) + } + } + } +} + +func TestMonitorStartedAfterAMockModeSwitchLeavesTheModeItWasBuiltIn(t *testing.T) { + setMockSamplerTestEnv(t, time.Hour, 5*time.Minute) + pinDefaultMockEstate(t) + manager := newMockHostAlertTestManager(t) + monitor := &Monitor{ + state: models.NewState(), + alertManager: manager, + metricsHistory: NewMetricsHistory(10, time.Hour), + } + // Built in live mode: New restored this live alert from disk. + manager.CheckNode(models.Node{ + ID: "live-node", + Name: "live-node", + Instance: "live", + Status: "online", + Memory: models.Memory{Total: 64 << 30, Used: 60 << 30, Free: 4 << 30, Usage: 93.75}, + }) + if len(manager.GetActiveAlerts()) == 0 { + t.Fatal("live node raised no alert") + } + + // Another monitor switched to mock mode before this one's Start joined. + mustSetMockEnabled(t, true) + started := false + leave := monitor.joinMockModeSwitches(func(mockEnabled bool) { started = mockEnabled }) + defer leave() + + if !started { + t.Fatal("Start was not handed mock mode") + } + if active := manager.GetActiveAlerts(); len(active) != 0 { + t.Fatalf("a monitor built in live mode started in mock mode with its live alerts: %s", describeAlerts(active)) + } + mockModeSwitchMu.Lock() + _, running := mockModeRunningMonitors[monitor] + mockModeSwitchMu.Unlock() + if !running { + t.Fatal("joined monitor is not reached by later switches") + } +} + +func TestMonitorStartedAfterAMockModeRoundTripDropsWhatItRaisedMeanwhile(t *testing.T) { + setMockSamplerTestEnv(t, time.Hour, 5*time.Minute) + pinDefaultMockEstate(t) + newMonitor := func() *Monitor { + return &Monitor{ + state: models.NewState(), + alertManager: newMockHostAlertTestManager(t), + metricsHistory: NewMetricsHistory(10, time.Hour), + } + } + switcher, starting := newMonitor(), newMonitor() + // Built in live mode, as New records it, and not running yet. + mockModeSwitchMu.Lock() + starting.mockModeSwitchesSeen = mockModeSwitchCount.Load() + mockModeSwitchMu.Unlock() + + mustSetMonitorMockMode(t, switcher, true) + // A read path evaluates fixture data on the monitor before its Start runs. + fixture := mock.CurrentFixtureGraph().State.Nodes[0] + fixture.Status = "online" + fixture.Memory = models.Memory{Total: 64 << 30, Used: 60 << 30, Free: 4 << 30, Usage: 93.75} + starting.alertManager.CheckNode(fixture) + if len(starting.alertManager.GetActiveAlerts()) == 0 { + t.Fatal("fixture node raised no alert") + } + mustSetMonitorMockMode(t, switcher, false) + + leave := starting.joinMockModeSwitches(func(bool) {}) + defer leave() + if active := starting.alertManager.GetActiveAlerts(); len(active) != 0 { + t.Fatalf("a monitor that missed a mock-mode round trip started in live mode with fixture alerts: %s", describeAlerts(active)) + } +} + +func TestSetMockModeAlignsAMonitorWhoseModeTheFlagAlreadyHolds(t *testing.T) { + setMockSamplerTestEnv(t, time.Hour, 5*time.Minute) + pinDefaultMockEstate(t) + manager := newMockHostAlertTestManager(t) + monitor := &Monitor{ + state: models.NewState(), + alertManager: manager, + metricsHistory: NewMetricsHistory(10, time.Hour), + } + // Built in live mode, with a live alert, and not running yet. + manager.CheckNode(models.Node{ + ID: "live-node", + Name: "live-node", + Instance: "live", + Status: "online", + Memory: models.Memory{Total: 64 << 30, Used: 60 << 30, Free: 4 << 30, Usage: 93.75}, + }) + if len(manager.GetActiveAlerts()) == 0 { + t.Fatal("live node raised no alert") + } + + // Another monitor's switch already set the flag. + mustSetMockEnabled(t, true) + mustSetMonitorMockMode(t, monitor, true) + + if active := manager.GetActiveAlerts(); len(active) != 0 { + t.Fatalf("SetMockMode(true) left a monitor built in live mode with its live alerts: %s", describeAlerts(active)) + } +} + +func TestMockModeSwitchLeavesAnUnjoinedMonitorsRuntimeToItsStart(t *testing.T) { + setMockSamplerTestEnv(t, time.Hour, 5*time.Minute) + pinDefaultMockEstate(t) + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + // Start has set its runtime context but not joined the switches yet. + monitor := &Monitor{ + config: &config.Config{}, + state: models.NewState(), + alertManager: newMockHostAlertTestManager(t), + metricsHistory: NewMetricsHistory(10, time.Hour), + runtimeCtx: ctx, + } + t.Cleanup(monitor.stopMockMetricsSampler) + + mustSetMonitorMockMode(t, monitor, true) + monitor.mu.RLock() + started := monitor.mockMetricsCancel != nil + monitor.mu.RUnlock() + if started { + t.Fatal("a switch started the runtime of a monitor whose Start has not joined; that runtime belongs to its Start") + } + + leave := monitor.joinMockModeSwitches(func(mockEnabled bool) { + monitor.startModeRuntime(ctx, nil, mockEnabled) + }) + defer leave() + monitor.mu.RLock() + started = monitor.mockMetricsCancel != nil + monitor.mu.RUnlock() + if !started { + t.Fatal("joining in mock mode did not start the mock sampler") + } +} + // diskAlertKeys lists the active disk-health and disk-wearout alerts as // "type instance/node device" so assertions name the disk, not the alert ID. func diskAlertKeys(manager *alerts.Manager) map[string]bool { From 447807526fc07e4521f61b8730a6422e8f381ea3 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 13:58:33 +0100 Subject: [PATCH 5/9] Show the outdated sensor setup notice on every Proxmox tab The Proxmox page warns when a node's SSH temperature monitoring still runs the pre-rc.6 setup, which locks the key to `sensors -j` so SATA/SAS disk temperatures never arrive. The notice judged that from the disk rows of the active tab, and since each tab hydrates only its own resource families, Overview loads no disks and never showed it. The registry now derives the verdict on the node itself, after child counts resolve each disk's canonical parent: a node with an available, legacy-format temperature payload is flagged when a SMART-only disk under it has no reading collected now. The page lists flagged nodes from the rows every tab already loads, so Overview needs no disk inventory. Every Proxmox node carries an explicit true or false, because the browser merges a facet field by field and would keep an earlier true if a cleared verdict were omitted. Two rule fixes ride along with the move. Proxmox's own disk inventory types SATA/SAS disks as hdd or ssd, so the old sata/sas-only set never matched a node without a host agent. And a retained reading kept after a disk stopped being read no longer counts as present (diskinventory.TemperatureCollected). --- .../subsystems/frontend-primitives.md | 15 +- .../internal/subsystems/storage-recovery.md | 9 + .../internal/subsystems/unified-resources.md | 30 +++ .../15418f6d41666705.json | 46 ++++ .../sensorSetup.branchcov0712.test.ts | 208 --------------- .../features/platformPage/sensorSetup.test.ts | 147 ++--------- .../src/features/platformPage/sensorSetup.ts | 48 +--- .../features/proxmox/ProxmoxPageSurface.tsx | 7 +- .../ProxmoxPageSurface.contract.test.tsx | 38 +++ .../__tests__/useUnifiedResources.test.ts | 6 +- .../src/hooks/useUnifiedResources.ts | 1 + frontend-modern/src/types/resource.ts | 4 + .../__tests__/resourceStateAdapters.test.ts | 23 ++ internal/unifiedresources/clone.go | 1 + internal/unifiedresources/clone_test.go | 19 ++ .../unifiedresources/proxmox_sensor_setup.go | 79 ++++++ internal/unifiedresources/registry.go | 3 + internal/unifiedresources/registry_test.go | 245 ++++++++++++++++++ internal/unifiedresources/types.go | 8 + 19 files changed, 559 insertions(+), 378 deletions(-) create mode 100644 frontend-modern/browser-verification/15418f6d41666705.json delete mode 100644 frontend-modern/src/features/platformPage/__tests__/sensorSetup.branchcov0712.test.ts create mode 100644 internal/unifiedresources/proxmox_sensor_setup.go diff --git a/docs/release-control/v6/internal/subsystems/frontend-primitives.md b/docs/release-control/v6/internal/subsystems/frontend-primitives.md index d837f96e3d..6a3b9128bd 100644 --- a/docs/release-control/v6/internal/subsystems/frontend-primitives.md +++ b/docs/release-control/v6/internal/subsystems/frontend-primitives.md @@ -6661,6 +6661,11 @@ icon/content layout, and action-link chrome. The outdated-sensor notices, and the `platform-inline-notice-local-amber-shell` pattern guard blocks future `platformPage` files from reintroducing page-local amber notice shells. +A notice's render predicate reads what its page hydrates on every tab the +notice renders on. `collectOutdatedSensorSetupNodes` lists the PVE nodes that +carry the registry's `proxmox.sensorSetupOutdated` verdict and takes no disk +rows, because only the Proxmox Storage tab loads them; a predicate over them +left the notice silent on Overview. Alert incident-event filter containers, labels, and chips must now route through the shared presentation helpers in @@ -7896,10 +7901,12 @@ renders above the chart. TrueNAS disk rows follow it too: the drawer's Temperature row reads "(last known)" with the reason as title and no heat tone, and the one-line summary leaves a retained reading out because it has no room for the reason. -The Proxmox outdated sensor setup notice (`features/platformPage/sensorSetup.ts`) -counts a SATA or SAS disk as having a temperature only when the reading is -current, so a retained one cannot hide the notice for a disk Pulse cannot read -now. +The Proxmox outdated sensor setup notice reads the registry's per-node +`proxmox.sensorSetupOutdated` verdict, which counts a SATA or SAS disk as having +a temperature only when the reading was collected now +(`diskinventory.TemperatureCollected` in +`internal/unifiedresources/proxmox_sensor_setup.go`), so a retained one cannot +hide the notice for a disk Pulse cannot read now. The focused browser proofs are `frontend-modern/src/features/patrol/__tests__/patrolRunAcceptance.test.ts`, diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index 66185fb88d..91e8334be9 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -944,6 +944,15 @@ Patrol coverage, schedule, finding, and approval state belongs on Patrol-owned surfaces or explicit Patrol affordances; Proxmox overview must not treat it as backup coverage, restore readiness, PBS verification/protection proof, or a replacement for the Proxmox Backups tab and workload Backup column. +The outdated sensor setup notice above the Proxmox tabs is temperature +guidance, not storage or recovery evidence. It reads the registry's per-node +`proxmox.sensorSetupOutdated` verdict from the node rows the active tab already +loads (Overview, Storage, Replication and Backups load nodes; Ceph and Mail do +not, so it does not render there), so the Overview query must not add +`physical_disk` rows to feed it; the derived `ProxmoxData` flag opens no storage +or recovery path. +`ProxmoxPageSurface.contract.test.tsx` pins the notice on Overview with no disk +rows and an Overview query without `physical_disk`. The Proxmox inline estate totals are likewise an adjacent frontend-primitives and unified-resource projection, not backup or recovery evidence. `ProxmoxPageSurface` must flow the already-loaded workload inventory into the diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index 25fb124809..1141ff711f 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -2997,6 +2997,36 @@ or force every provider type back into the active route payload. Docker, Kubernetes, TrueNAS, and VMware surface contract tests pin their route query maps, and the shared hook test pins facet normalization. +### Proxmox node verdicts over disks arrive on the node + +The Proxmox page hydrates one query per workflow, and only its Storage tab +loads `physical_disk` rows, so a node verdict that depends on the node's disks +is derived in the registry and carried on the node, never rebuilt in the page +from disk rows. `ProxmoxData.SensorSetupOutdated` (`proxmox.sensorSetupOutdated` +on REST and websocket rows) is that verdict for the outdated sensor setup +notice. `refreshProxmoxSensorSetupLocked` +(`internal/unifiedresources/proxmox_sensor_setup.go`) runs after +`buildChildCounts` on every ingest path and gives every Proxmox node an +explicit true or false (other resources omit it), because browser facet merges +read an omitted field as a partial snapshot and would keep an earlier true. It +is true when the node's temperature payload is available and in the legacy +`sensors -j` format (`models.Temperature.LegacySensorsFormat`) and a disk whose +canonical parent is the node has no reading collected now +(`diskinventory.TemperatureCollected`) and a type whose temperature a PVE node +gets only through SMART: `sata`, `sas`, or the `hdd` and `ssd` form factors +Proxmox's own inventory reports. Proxmox also types a non-rotational USB device +`ssd`, so a USB device without SMART counts. A retained reading does not count +as current, including one kept while a host agent reports the disk in standby; +a reading a linked host agent collects now clears the disk. NVMe disks never +set it, because kernel hwmon reports them even on a legacy setup, and a host +agent the registry has not linked to the node contributes no disks. The +verdict reflects the registry's node and disk state at each ingest. +`TestProxmoxNodeSensorSetupOutdatedFromItsDisks` and +`TestProxmoxNodeSensorSetupOutdatedFollowsWhereTheReadingArrives` in +`internal/unifiedresources/registry_test.go` pin the rule, the linked-agent +case, the explicit false on the wire and the presented JSON; +`resourceStateAdapters.test.ts` pins that a false clears a merged true. + ### Canonical REST facets preserve realtime workload evidence The frontend REST projection retains the complete source-authored Proxmox diff --git a/frontend-modern/browser-verification/15418f6d41666705.json b/frontend-modern/browser-verification/15418f6d41666705.json new file mode 100644 index 0000000000..2ea8e947d3 --- /dev/null +++ b/frontend-modern/browser-verification/15418f6d41666705.json @@ -0,0 +1,46 @@ +{ + "version": 1, + "base_sha": "e02edd9989c88fbe6e5459afdf488c73538ddd24", + "verified_at": "2026-10-07T12:56:24Z", + "result": "passed", + "changed_paths": [ + "frontend-modern/src/features/platformPage/sensorSetup.ts", + "frontend-modern/src/features/proxmox/ProxmoxPageSurface.tsx", + "frontend-modern/src/hooks/useUnifiedResources.ts", + "frontend-modern/src/types/resource.ts" + ], + "content_sha256": { + "frontend-modern/src/features/platformPage/sensorSetup.ts": "18e4da01eb9206bce83a2e7d2fbb9f97313685c5655d620092a9e0ca51804ef5", + "frontend-modern/src/features/proxmox/ProxmoxPageSurface.tsx": "db074960c69c59bb030814a04028a50ee5b347b7cf16b04b4c6edea708ffd782", + "frontend-modern/src/hooks/useUnifiedResources.ts": "77cc56a11213f951e051ceabd05bc2bf3161adedadc3586f2badee98bf1056c3", + "frontend-modern/src/types/resource.ts": "37e3ddb2e1334472a280e0ef93053292002567acabf84aace9fed1735a763b57" + }, + "routes": [ + "/proxmox", + "/proxmox/storage", + "/proxmox/replication", + "/proxmox/backups" + ], + "viewports": [ + { + "width": 1440, + "height": 900 + }, + { + "width": 390, + "height": 844 + } + ], + "states": [ + "Private mock stack (8-node estate) built from this tree with a local-only generator overlay: pve2, pve3 and pve5 report the legacy sensors -j payload; pve2 has a Proxmox-inventory hdd disk with no temperature, pve3 a SATA disk whose reading is retained (host agent stopped reporting), pve5 a SATA disk with no temperature.", + "Overview, Storage, Replication and Backups each show the warning notice naming pve2, pve3 and pve5 at both viewports. The Overview query is type=agent,vm,system-container,oci-container and no physical_disk request is made on Overview, Replication or Backups; only Storage requests disk rows.", + "Before control on the same stack with origin/main's ProxmoxPageSurface.tsx and sensorSetup.ts swapped in: no notice on Overview, Replication or Backups, and Storage names only pve5 (hdd type and retained reading missed).", + "Websocket frames carry sensorSetupOutdated true on every tab. Fresh desktop load: no console errors and no 5xx responses. Notice text fits the 390px viewport without overflow; the node table stays below it in reading order.", + "Mock data only; not a real pre-rc.6 SSH node, installed upgrade or release acceptance." + ], + "interactions": [ + "Navigated each tab by URL at 1440x900 and 390x844 and waited 20s on Overview to outlast websocket row merges; the notice stayed.", + "Live clearing without reload: switching pve5's disk to a current 37C reading dropped pve5 from the notice (3 nodes to pve2, pve3) on the next websocket update. Control with the earlier omitempty backend kept pve5 for 90s.", + "Open Infrastructure settings link targets /settings/infrastructure (contract test)." + ] +} diff --git a/frontend-modern/src/features/platformPage/__tests__/sensorSetup.branchcov0712.test.ts b/frontend-modern/src/features/platformPage/__tests__/sensorSetup.branchcov0712.test.ts deleted file mode 100644 index ce1e3a6d06..0000000000 --- a/frontend-modern/src/features/platformPage/__tests__/sensorSetup.branchcov0712.test.ts +++ /dev/null @@ -1,208 +0,0 @@ -import { describe, expect, it } from 'vitest'; -import type { Resource } from '@/types/resource'; -import { collectOutdatedSensorSetupNodes } from '../sensorSetup'; - -// Branch-coverage tests for the two UNEXPORTED helpers inside sensorSetup.ts: -// - getNodeName (3-way `||` fallback chain + `.trim()`) -// - isSMARTOnlyDiskWithoutTemperature (no-meta guard, diskType normalize, -// SMART-set membership, `temperature ?? 0`) -// Both helpers are private, so every case drives them through the public -// `collectOutdatedSensorSetupNodes` and asserts the concrete observable shape. -// -// Conventions mirror the sibling `sensorSetup.test.ts` fixtures. Disks are -// linked to nodes either by `platformData.proxmox.nodeName` (the name-matching -// path, used to prove `getNodeName` resolution) or by `parentId` (which makes -// `matchesPhysicalDiskNode` short-circuit true, used to isolate the SMART-only -// predicate from name resolution). - -type LegacyNodeOver = { - id?: string; - name?: string; - proxmox?: Partial; -}; - -const legacyNode = (over: LegacyNodeOver = {}): Resource => - ({ - id: over.id ?? 'node-1', - name: over.name ?? '', - displayName: over.name ?? '', - type: 'node', - proxmox: { - temperatureDetails: { available: true, legacySensorsFormat: true }, - ...(over.proxmox ?? {}), - }, - }) as unknown as Resource; - -type DiskOver = { - id?: string; - parentId?: string; - nodeName?: string; - diskType?: string; - temperature?: number; - noMeta?: boolean; -}; - -const disk = (over: DiskOver = {}): Resource => - ({ - id: over.id ?? 'disk-1', - name: 'disk', - displayName: 'disk', - type: 'physical_disk', - parentId: over.parentId, - physicalDisk: over.noMeta - ? undefined - : { - diskType: over.diskType, - temperature: over.temperature, - }, - platformData: over.nodeName ? { proxmox: { nodeName: over.nodeName } } : {}, - }) as unknown as Resource; - -describe('getNodeName (via collectOutdatedSensorSetupNodes)', () => { - it('falls back to proxmox.nodeName when proxmox.node is absent', () => { - const node = legacyNode({ id: 'n-nodeName', proxmox: { nodeName: 'bravo' } }); - const result = collectOutdatedSensorSetupNodes( - [node], - // Disk resolves to the node by the nodeName-derived identity, proving - // getNodeName returned 'bravo' (not '' and not node.name). - [disk({ id: 'd1', nodeName: 'bravo', diskType: 'sata', temperature: 0 })], - ); - expect(result).toStrictEqual([{ id: 'n-nodeName', name: 'bravo' }]); - }); - - it('falls back to the resource name when both proxmox.node and nodeName are absent', () => { - const node = legacyNode({ id: 'n-name', name: 'charlie' }); - const result = collectOutdatedSensorSetupNodes( - [node], - [disk({ id: 'd2', nodeName: 'charlie', diskType: 'sata', temperature: 0 })], - ); - expect(result).toStrictEqual([{ id: 'n-name', name: 'charlie' }]); - }); - - it('trims surrounding whitespace from proxmox.node before using it as the node identity', () => { - const node = legacyNode({ id: 'n-trim', proxmox: { node: ' delta ' } }); - const result = collectOutdatedSensorSetupNodes( - [node], - // matchesPhysicalDiskNode lowercases/trims both sides, so 'delta' must be - // the trimmed getNodeName output for the disk to match. - [disk({ id: 'd3', nodeName: 'delta', diskType: 'sata', temperature: 0 })], - ); - expect(result).toStrictEqual([{ id: 'n-trim', name: 'delta' }]); - }); - - it('returns the empty string when every name source is blank, observable via a parentId-linked disk', () => { - // No proxmox.node, no proxmox.nodeName, name is whitespace -> getNodeName - // hits the final `|| ''` arm and trims to ''. The disk is matched by - // parentId so matchesPhysicalDiskNode short-circuits true regardless of the - // (empty) name, letting the flagged node surface with name: ''. - const node = legacyNode({ id: 'n-empty', name: ' ' }); - const result = collectOutdatedSensorSetupNodes( - [node], - [disk({ id: 'd4', parentId: 'n-empty', diskType: 'sata', temperature: 0 })], - ); - expect(result).toStrictEqual([{ id: 'n-empty', name: '' }]); - }); -}); - -describe('isSMARTOnlyDiskWithoutTemperature (via collectOutdatedSensorSetupNodes)', () => { - it('returns false for a disk with no physicalDisk metadata, so the node is not flagged', () => { - // Disk is linked to the node by parentId (match guaranteed true); the only - // reason the node drops out is the `if (!meta) return false` guard. - const node = legacyNode({ id: 'n-nometa', proxmox: { node: 'pve' } }); - const result = collectOutdatedSensorSetupNodes( - [node], - [disk({ id: 'd-nometa', parentId: 'n-nometa', noMeta: true })], - ); - expect(result).toStrictEqual([]); - }); - - it('returns false when diskType is missing, so a matched disk does not flag the node', () => { - // diskType undefined -> `(meta.diskType || '')` -> '' not in the SMART-only - // set. parentId link isolates the cause to the diskType check. - const node = legacyNode({ id: 'n-notype', proxmox: { node: 'pve' } }); - const result = collectOutdatedSensorSetupNodes( - [node], - [disk({ id: 'd-notype', parentId: 'n-notype', temperature: 0 })], - ); - expect(result).toStrictEqual([]); - }); - - it('normalizes a whitespace-padded uppercase SAS diskType and flags the node', () => { - // Exercises `diskType.trim().toLowerCase()` -> 'sas' (in the SMART-only - // set) and temperature 0 -> `(0 ?? 0) <= 0` true. - const node = legacyNode({ id: 'n-sas', proxmox: { node: 'pve' } }); - const result = collectOutdatedSensorSetupNodes( - [node], - [ - disk({ - id: 'd-sas', - parentId: 'n-sas', - diskType: ' SAS ', - temperature: 0, - }), - ], - ); - expect(result).toStrictEqual([{ id: 'n-sas', name: 'pve' }]); - }); - - it('uses 0 in place of an undefined temperature and flags the node', () => { - // temperature is omitted entirely -> `(meta.temperature ?? 0)` takes the - // nullish right-hand side (0) -> `<= 0` true. - const node = legacyNode({ id: 'n-undef-temp', proxmox: { node: 'pve' } }); - const result = collectOutdatedSensorSetupNodes( - [node], - [disk({ id: 'd-undef-temp', parentId: 'n-undef-temp', diskType: 'sata' })], - ); - expect(result).toStrictEqual([{ id: 'n-undef-temp', name: 'pve' }]); - }); - - it('uses 0 in place of an explicit null temperature and flags the node', () => { - // Deliberately-malformed payload: temperature typed as number | undefined - // but carrying null. The `?? 0` coalesces it to 0 -> `<= 0` true. - const node = legacyNode({ id: 'n-null-temp', proxmox: { node: 'pve' } }); - const nullTempDisk = { - id: 'd-null-temp', - name: 'disk', - displayName: 'disk', - type: 'physical_disk', - parentId: 'n-null-temp', - physicalDisk: { diskType: 'sata', temperature: null }, - } as unknown as Resource; - const result = collectOutdatedSensorSetupNodes([node], [nullTempDisk]); - expect(result).toStrictEqual([{ id: 'n-null-temp', name: 'pve' }]); - }); - - it('flags the node for a SATA disk reporting a negative temperature', () => { - // Concrete negative value exercises the `<= 0` true arm (not the ?? arm). - const node = legacyNode({ id: 'n-neg-temp', proxmox: { node: 'pve' } }); - const result = collectOutdatedSensorSetupNodes( - [node], - [disk({ id: 'd-neg-temp', parentId: 'n-neg-temp', diskType: 'sata', temperature: -5 })], - ); - expect(result).toStrictEqual([{ id: 'n-neg-temp', name: 'pve' }]); - }); - - it('does not flag the node when a SMART-only disk reports a positive temperature', () => { - // SAS at 41 degrees -> `(41 ?? 0) <= 0` is false. - const node = legacyNode({ id: 'n-warm', proxmox: { node: 'pve' } }); - const result = collectOutdatedSensorSetupNodes( - [node], - [disk({ id: 'd-warm', parentId: 'n-warm', diskType: 'sas', temperature: 41 })], - ); - expect(result).toStrictEqual([]); - }); - - it('evaluates the no-meta guard on the first disk but still flags the node via a second matching disk', () => { - // Drives the `!meta` branch on d-bad, then `.some()` advances to d-good - // which is SMART-only without a temperature -> node flagged. - const node = legacyNode({ id: 'n-mixed', proxmox: { node: 'pve' } }); - const result = collectOutdatedSensorSetupNodes( - [node], - [ - disk({ id: 'd-bad', parentId: 'n-mixed', noMeta: true }), - disk({ id: 'd-good', parentId: 'n-mixed', diskType: 'sata', temperature: 0 }), - ], - ); - expect(result).toStrictEqual([{ id: 'n-mixed', name: 'pve' }]); - }); -}); diff --git a/frontend-modern/src/features/platformPage/sensorSetup.test.ts b/frontend-modern/src/features/platformPage/sensorSetup.test.ts index 3b2ada61d9..949c649186 100644 --- a/frontend-modern/src/features/platformPage/sensorSetup.test.ts +++ b/frontend-modern/src/features/platformPage/sensorSetup.test.ts @@ -1,150 +1,57 @@ import { describe, expect, it } from 'vitest'; -import type { PhysicalDiskCollectionStatus, Resource } from '@/types/resource'; +import type { Resource } from '@/types/resource'; import { collectOutdatedSensorSetupNodes } from './sensorSetup'; const node = (over: Partial & { proxmox?: Resource['proxmox'] }): Resource => - ({ id: 'node-1', name: 'pve1', type: 'node', ...over }) as Resource; + ({ id: 'node-1', name: 'pve1', type: 'agent', ...over }) as Resource; -const disk = (over: { - id?: string; - parentId?: string; - nodeName?: string; - diskType?: string; - temperature?: number; - collection?: PhysicalDiskCollectionStatus; -}): Resource => - ({ - id: over.id ?? 'disk-1', - name: 'disk', - type: 'physical_disk', - parentId: over.parentId, - physicalDisk: { - diskType: over.diskType ?? 'sata', - temperature: over.temperature ?? 0, - collection: over.collection, - }, - platformData: over.nodeName ? { proxmox: { nodeName: over.nodeName } } : {}, - }) as unknown as Resource; - -const legacyNode = (over: Partial = {}): Resource => +const flaggedNode = (over: Partial = {}): Resource => node({ proxmox: { node: 'pve1', temperatureDetails: { available: true, legacySensorsFormat: true }, + sensorSetupOutdated: true, }, ...over, }); describe('collectOutdatedSensorSetupNodes', () => { - it('flags a legacy-format node with a SATA disk missing a temperature', () => { - const result = collectOutdatedSensorSetupNodes( - [legacyNode()], - [disk({ nodeName: 'pve1', diskType: 'sata' })], - ); - expect(result).toEqual([{ id: 'node-1', name: 'pve1' }]); + it('lists a node carrying the registry verdict without any disk rows', () => { + expect(collectOutdatedSensorSetupNodes([flaggedNode()])).toEqual([ + { id: 'node-1', name: 'pve1' }, + ]); }); - it('matches disks to nodes by parentId as well as node name', () => { - const result = collectOutdatedSensorSetupNodes( - [legacyNode()], - [disk({ parentId: 'node-1', diskType: 'sas' })], - ); - expect(result).toEqual([{ id: 'node-1', name: 'pve1' }]); - }); - - it('ignores nodes whose payload is the wrapper format', () => { - const wrapperNode = node({ - proxmox: { node: 'pve1', temperatureDetails: { available: true } }, - }); - expect(collectOutdatedSensorSetupNodes([wrapperNode], [disk({ nodeName: 'pve1' })])).toEqual( - [], - ); - }); - - it('ignores legacy nodes when temperature collection did not succeed', () => { - const unavailableNode = node({ + it('leaves a legacy-format node the registry did not flag', () => { + // The registry clears the verdict when every SATA/SAS disk under the node + // has a current temperature, for example from a linked host agent. + const reading = node({ proxmox: { node: 'pve1', - temperatureDetails: { available: false, legacySensorsFormat: true }, + temperatureDetails: { available: true, legacySensorsFormat: true }, }, }); - expect( - collectOutdatedSensorSetupNodes([unavailableNode], [disk({ nodeName: 'pve1' })]), - ).toEqual([]); + expect(collectOutdatedSensorSetupNodes([reading])).toEqual([]); }); - it('ignores legacy nodes whose SATA/SAS disks already have temperatures', () => { - expect( - collectOutdatedSensorSetupNodes( - [legacyNode()], - [disk({ nodeName: 'pve1', diskType: 'sata', temperature: 34 })], - ), - ).toEqual([]); - }); - - it('treats a retained temperature that is not current as missing', () => { - // Normalization keeps a disk's last known temperature when the current - // observation did not collect one, such as after a linked host agent - // stopped reporting. The disk cannot be read now, so the notice stays. - const retained = (state: 'available' | 'unavailable' | 'missing') => - disk({ - nodeName: 'pve1', - diskType: 'sata', - temperature: 34, - collection: { - temperature: { state, source: 'agent', reason: 'host agent stopped reporting' }, - }, - }); - expect(collectOutdatedSensorSetupNodes([legacyNode()], [retained('unavailable')])).toEqual([ - { id: 'node-1', name: 'pve1' }, + it('names a node by its Proxmox node, then node name, then resource name, trimmed', () => { + const nodes = [ + node({ id: 'by-node', proxmox: { node: ' alpha ', sensorSetupOutdated: true } }), + node({ id: 'by-node-name', proxmox: { nodeName: 'bravo', sensorSetupOutdated: true } }), + node({ id: 'by-name', name: 'charlie', proxmox: { sensorSetupOutdated: true } }), + ]; + expect(collectOutdatedSensorSetupNodes(nodes)).toEqual([ + { id: 'by-node', name: 'alpha' }, + { id: 'by-node-name', name: 'bravo' }, + { id: 'by-name', name: 'charlie' }, ]); - expect(collectOutdatedSensorSetupNodes([legacyNode()], [retained('missing')])).toEqual([ - { id: 'node-1', name: 'pve1' }, - ]); - expect(collectOutdatedSensorSetupNodes([legacyNode()], [retained('available')])).toEqual([]); - }); - - it('ignores legacy nodes with only NVMe disks missing temperatures', () => { - expect( - collectOutdatedSensorSetupNodes( - [legacyNode()], - [disk({ nodeName: 'pve1', diskType: 'nvme' })], - ), - ).toEqual([]); - }); - - it('ignores disks that belong to a different node', () => { - expect(collectOutdatedSensorSetupNodes([legacyNode()], [disk({ nodeName: 'pve2' })])).toEqual( - [], - ); }); it('sorts multiple affected nodes by name', () => { const nodes = [ - node({ - id: 'node-b', - name: 'pve-b', - proxmox: { - node: 'pve-b', - temperatureDetails: { available: true, legacySensorsFormat: true }, - }, - }), - node({ - id: 'node-a', - name: 'pve-a', - proxmox: { - node: 'pve-a', - temperatureDetails: { available: true, legacySensorsFormat: true }, - }, - }), + flaggedNode({ id: 'node-b', proxmox: { node: 'pve-b', sensorSetupOutdated: true } }), + flaggedNode({ id: 'node-a', proxmox: { node: 'pve-a', sensorSetupOutdated: true } }), ]; - const disks = [ - disk({ id: 'disk-a', nodeName: 'pve-a' }), - disk({ id: 'disk-b', nodeName: 'pve-b' }), - ]; - expect(collectOutdatedSensorSetupNodes(nodes, disks).map((n) => n.name)).toEqual([ - 'pve-a', - 'pve-b', - ]); + expect(collectOutdatedSensorSetupNodes(nodes).map((n) => n.name)).toEqual(['pve-a', 'pve-b']); }); }); diff --git a/frontend-modern/src/features/platformPage/sensorSetup.ts b/frontend-modern/src/features/platformPage/sensorSetup.ts index af8ff285b2..2e27d70d5a 100644 --- a/frontend-modern/src/features/platformPage/sensorSetup.ts +++ b/frontend-modern/src/features/platformPage/sensorSetup.ts @@ -1,10 +1,4 @@ import type { Resource } from '@/types/resource'; -import { matchesPhysicalDiskNode } from '@/components/Storage/diskResourceUtils'; -import { isPhysicalDiskTemperatureCurrent } from '@/features/storageBackups/diskTemperaturePresentation'; - -// Disk transports whose temperatures only arrive via SMART (the pulse-sensors -// wrapper). NVMe temps come from kernel hwmon and work even on legacy setups. -const SMART_ONLY_DISK_TYPES = new Set(['sata', 'sas']); export type OutdatedSensorSetupNode = { id: string; @@ -14,44 +8,16 @@ export type OutdatedSensorSetupNode = { const getNodeName = (node: Resource): string => (node.proxmox?.node || node.proxmox?.nodeName || node.name || '').trim(); -const isSMARTOnlyDiskWithoutTemperature = (disk: Resource): boolean => { - const meta = disk.physicalDisk; - if (!meta) return false; - const diskType = (meta.diskType || '').trim().toLowerCase(); - if (!SMART_ONLY_DISK_TYPES.has(diskType)) return false; - // A last known temperature kept for a disk that could not be read now, such - // as after a linked host agent stopped reporting, is not one arriving. - return (meta.temperature ?? 0) <= 0 || !isPhysicalDiskTemperatureCurrent(meta.collection); -}; - -// Flags PVE nodes whose SSH temperature monitoring still runs the pre-rc.6 +// Lists PVE nodes whose SSH temperature monitoring still runs the pre-rc.6 // setup (authorized_keys locked to `sensors -j`). Such a payload parses fine // and delivers CPU/NVMe temps, but SMART (SATA/SAS) disk temperatures can -// never arrive, so they silently stay blank. Data-gated three ways: the last -// collection succeeded, the payload was legacy-format, and the node actually -// has SATA/SAS disks sitting without a temperature. Nodes whose disk temps -// arrive elsewhere (e.g. a linked host agent) fall out via the last check. -export function collectOutdatedSensorSetupNodes( - nodes: Resource[], - physicalDisks: Resource[], -): OutdatedSensorSetupNode[] { - if (!nodes.length || !physicalDisks.length) return []; - +// never arrive, so they silently stay blank. The registry derives the verdict +// on the node (`proxmox.sensorSetupOutdated`) from the node's last collection, +// its legacy payload and the disks under it, so every Proxmox tab that lists +// nodes can show the notice without loading the disk inventory. +export function collectOutdatedSensorSetupNodes(nodes: Resource[]): OutdatedSensorSetupNode[] { return nodes - .filter((node) => { - const details = node.proxmox?.temperatureDetails; - if (!details?.available || !details.legacySensorsFormat) return false; - const nodeName = getNodeName(node); - return physicalDisks.some( - (disk) => - isSMARTOnlyDiskWithoutTemperature(disk) && - matchesPhysicalDiskNode(disk, { - id: node.id, - name: nodeName, - instance: node.proxmox?.instance, - }), - ); - }) + .filter((node) => node.proxmox?.sensorSetupOutdated === true) .map((node) => ({ id: node.id, name: getNodeName(node) })) .sort((a, b) => a.name.localeCompare(b.name)); } diff --git a/frontend-modern/src/features/proxmox/ProxmoxPageSurface.tsx b/frontend-modern/src/features/proxmox/ProxmoxPageSurface.tsx index e34418e333..fa0a193e29 100644 --- a/frontend-modern/src/features/proxmox/ProxmoxPageSurface.tsx +++ b/frontend-modern/src/features/proxmox/ProxmoxPageSurface.tsx @@ -286,11 +286,10 @@ export function ProxmoxPageSurface() { const serverVersionDisplay = createMemo(() => formatAgentVersionDisplay(agentUpdateTargetVersion()), ); + // The node carries the registry's verdict, so the notice reads the same on + // every tab that lists nodes; no tab hydrates disks for it. const outdatedSensorSetupNodes = createMemo(() => - collectOutdatedSensorSetupNodes( - model().pveNodes, - model().resources.filter((resource) => resource.type === 'physical_disk'), - ), + collectOutdatedSensorSetupNodes(model().pveNodes), ); // The hosts table at the top and the embedded WorkloadsSurface below share diff --git a/frontend-modern/src/features/proxmox/__tests__/ProxmoxPageSurface.contract.test.tsx b/frontend-modern/src/features/proxmox/__tests__/ProxmoxPageSurface.contract.test.tsx index 198c6b273c..63964ff67a 100644 --- a/frontend-modern/src/features/proxmox/__tests__/ProxmoxPageSurface.contract.test.tsx +++ b/frontend-modern/src/features/proxmox/__tests__/ProxmoxPageSurface.contract.test.tsx @@ -375,6 +375,44 @@ describe('ProxmoxPageSurface contract', () => { expect(screen.queryByTestId('platform-outdated-agent-notice')).not.toBeInTheDocument(); }); + it('shows the outdated sensor setup notice on Overview without hydrating disks', () => { + const node = makeResource({ + id: 'agent:pve3', + name: 'pve3', + displayName: 'pve3', + type: 'agent', + proxmox: { + nodeName: 'pve3', + temperatureDetails: { available: true, legacySensorsFormat: true }, + sensorSetupOutdated: true, + }, + }); + // Like production, the Overview query returns nodes and guests only: the + // verdict has to arrive on the node, because no physical_disk rows do. + mockUseUnifiedResources.mockImplementation((options: { cacheKey?: string }) => ({ + resources: () => (options.cacheKey === 'proxmox-overview' ? [node] : []), + loading: () => false, + error: () => null, + refetch: vi.fn(), + })); + + renderSurface(); + + const overviewOptions = mockUseUnifiedResources.mock.calls + .map(([options]) => options as { cacheKey?: string; query?: string }) + .find((options) => options.cacheKey === 'proxmox-overview'); + expect(overviewOptions?.query).toBeDefined(); + expect(overviewOptions?.query).not.toContain('physical_disk'); + expect(screen.getByTestId('platform-section-tabs')).toHaveAttribute('data-active', 'overview'); + expect(screen.getByTestId('platform-outdated-sensor-setup-notice')).toHaveTextContent( + 'pve3 is using an older temperature monitoring setup that cannot read SATA/SAS disk temperatures.', + ); + expect(screen.getByRole('link', { name: 'Open Infrastructure settings' })).toHaveAttribute( + 'href', + '/settings/infrastructure', + ); + }); + it('folds estate topology into the existing nodes table header contract', () => { setResources([ makeResource({ diff --git a/frontend-modern/src/hooks/__tests__/useUnifiedResources.test.ts b/frontend-modern/src/hooks/__tests__/useUnifiedResources.test.ts index 3f62f9fa63..3d1f06a4ba 100644 --- a/frontend-modern/src/hooks/__tests__/useUnifiedResources.test.ts +++ b/frontend-modern/src/hooks/__tests__/useUnifiedResources.test.ts @@ -1943,7 +1943,7 @@ describe('useUnifiedResources', () => { dispose(); }); - it('passes proxmox temperatureDetails through for the outdated-sensor-setup gate', async () => { + it('passes the proxmox temperature payload and sensor setup verdict through', async () => { apiFetchMock.mockResolvedValueOnce({ ok: true, json: async () => ({ @@ -1956,6 +1956,7 @@ describe('useUnifiedResources', () => { nodeName: 'pve1', temperature: 58.4, temperatureDetails: { available: true, legacySensorsFormat: true }, + sensorSetupOutdated: true, }, }, ], @@ -1974,6 +1975,9 @@ describe('useUnifiedResources', () => { available: true, legacySensorsFormat: true, }); + // The outdated sensor setup notice reads the registry's verdict on the + // node, so it must survive the projection that rows on every tab go through. + expect(result!.resources()[0].proxmox?.sensorSetupOutdated).toBe(true); dispose(); }); diff --git a/frontend-modern/src/hooks/useUnifiedResources.ts b/frontend-modern/src/hooks/useUnifiedResources.ts index 489881c89c..6c6547e550 100644 --- a/frontend-modern/src/hooks/useUnifiedResources.ts +++ b/frontend-modern/src/hooks/useUnifiedResources.ts @@ -261,6 +261,7 @@ type APIResource = { available?: boolean; legacySensorsFormat?: boolean; }; + sensorSetupOutdated?: boolean; template?: boolean; containerType?: string; isOci?: boolean; diff --git a/frontend-modern/src/types/resource.ts b/frontend-modern/src/types/resource.ts index 0bc9b1826b..a80952ef36 100644 --- a/frontend-modern/src/types/resource.ts +++ b/frontend-modern/src/types/resource.ts @@ -773,6 +773,10 @@ export interface ResourceProxmoxMeta { available?: boolean; legacySensorsFormat?: boolean; }; + // Registry-derived on Proxmox nodes: the legacy setup above is in use and a + // SATA/SAS disk under the node has no current temperature. It arrives on the + // node so the outdated sensor setup notice never needs the disk inventory. + sensorSetupOutdated?: boolean; } export interface ResourceVirtualMachineMeta { diff --git a/frontend-modern/src/utils/__tests__/resourceStateAdapters.test.ts b/frontend-modern/src/utils/__tests__/resourceStateAdapters.test.ts index fa946df7c7..9673cfd24b 100644 --- a/frontend-modern/src/utils/__tests__/resourceStateAdapters.test.ts +++ b/frontend-modern/src/utils/__tests__/resourceStateAdapters.test.ts @@ -1379,6 +1379,29 @@ describe('resourceStateAdapters unavailable memory contract', () => { }); describe('incremental canonical resource snapshots', () => { + it('clears a node sensor setup verdict when the next snapshot sends false', () => { + // Facets merge field by field and an omitted field is read as a partial + // snapshot, so the registry sends every Proxmox node an explicit verdict. + const node = (sensorSetupOutdated?: boolean) => + ({ + id: 'agent-pve3', + type: 'agent', + name: 'pve3', + status: 'online', + sources: ['proxmox'], + proxmox: { nodeName: 'pve3', sensorSetupOutdated }, + }) as unknown as Resource; + + const [cleared] = mergeCanonicalResourceSnapshot([node(false)], [node(true)]); + expect(cleared?.proxmox?.sensorSetupOutdated).toBe(false); + + const [omitted] = mergeCanonicalResourceSnapshot( + [{ ...node(), proxmox: { nodeName: 'pve3' } } as unknown as Resource], + [node(true)], + ); + expect(omitted?.proxmox?.sensorSetupOutdated).toBe(true); + }); + it('preserves untouched row identity while refreshing changed resources', () => { const unchanged = { id: 'vm-unchanged', diff --git a/internal/unifiedresources/clone.go b/internal/unifiedresources/clone.go index 104c712f9f..0beca03f35 100644 --- a/internal/unifiedresources/clone.go +++ b/internal/unifiedresources/clone.go @@ -198,6 +198,7 @@ func cloneProxmoxData(in *ProxmoxData) *ProxmoxData { out.DockerCheckedAt = cloneTimePtr(in.DockerCheckedAt) out.TemperatureMonitoringEnabled = cloneBoolPtr(in.TemperatureMonitoringEnabled) out.PendingUpdatesCheckedAt = cloneTimePtr(in.PendingUpdatesCheckedAt) + out.SensorSetupOutdated = cloneBoolPtr(in.SensorSetupOutdated) return &out } diff --git a/internal/unifiedresources/clone_test.go b/internal/unifiedresources/clone_test.go index e46dc22840..63b87b54a1 100644 --- a/internal/unifiedresources/clone_test.go +++ b/internal/unifiedresources/clone_test.go @@ -518,6 +518,25 @@ func TestCloneProxmoxData_TemperatureIsolation(t *testing.T) { } } +// List and presentation readers get detached clones of a node, so the +// registry-derived sensor setup verdict travels with the clone and stays put +// when the live node's verdict changes afterwards. +func TestCloneProxmoxDataKeepsDerivedSensorSetupVerdict(t *testing.T) { + outdated := true + original := &ProxmoxData{NodeName: "pve", SensorSetupOutdated: &outdated} + cloned := cloneProxmoxData(original) + if cloned.SensorSetupOutdated == nil || !*cloned.SensorSetupOutdated { + t.Fatal("clone dropped the derived sensor setup verdict") + } + if cloned.SensorSetupOutdated == original.SensorSetupOutdated { + t.Fatal("clone shares the live node's verdict pointer") + } + *original.SensorSetupOutdated = false + if !*cloned.SensorSetupOutdated { + t.Fatal("changing the live node's verdict changed a clone already handed out") + } +} + func TestCloneProxmoxData_NetworkInterfaceIsolation(t *testing.T) { original := &ProxmoxData{ NetworkInterfaces: []NetworkInterface{ diff --git a/internal/unifiedresources/proxmox_sensor_setup.go b/internal/unifiedresources/proxmox_sensor_setup.go new file mode 100644 index 0000000000..4ff3c9c1b9 --- /dev/null +++ b/internal/unifiedresources/proxmox_sensor_setup.go @@ -0,0 +1,79 @@ +package unifiedresources + +import ( + "strings" + + "github.com/rcourtman/pulse-go-rewrite/pkg/diskinventory" +) + +// smartTemperatureDiskTypes are the disk types whose temperature reaches a +// Proxmox node only through SMART: the transports a host agent or the +// pulse-sensors wrapper reports (sata, sas) and the form factors Proxmox's own +// disk inventory reports for those same disks (hdd, ssd). Proxmox also types +// a non-rotational USB device ssd. NVMe temperatures come from kernel hwmon +// and arrive even on a legacy setup. +var smartTemperatureDiskTypes = map[string]struct{}{ + "sata": {}, + "sas": {}, + "hdd": {}, + "ssd": {}, +} + +// refreshProxmoxSensorSetupLocked derives ProxmoxData.SensorSetupOutdated on +// every Proxmox node, as an explicit true or false. SSH temperature monitoring +// set up before v6.0.0-rc.6 locks the node's key to `sensors -j`: the payload +// still parses and carries CPU and NVMe temperatures, but SMART disk +// temperatures can never arrive, so they stay blank with no error anywhere. +// The flag is data-gated three ways: the node's last temperature collection +// succeeded, its payload was the legacy format, and a disk under it waits on a +// SMART temperature it does not have now. A last known temperature kept for a +// disk that could not be read now (standby, a host agent past its lease) is not +// a current one (diskinventory.TemperatureCollected), while a disk whose +// temperature arrives another way, such as a linked host agent, falls out. +// Disks count through their canonical parent, so an agent the registry has not +// linked to the node contributes none. Deriving the flag here lets every +// surface that lists nodes read it without loading the node's disk inventory. +// It must run after buildChildCounts, which resolves each disk's canonical +// parent node. +func (rr *ResourceRegistry) refreshProxmoxSensorSetupLocked() { + waiting := make(map[string]struct{}) + for _, resource := range rr.resources { + if resource == nil { + continue + } + if resource.Proxmox != nil { + // A fresh pointer every time: a facet copied by an earlier merge or + // clone must not see this generation's verdict. + resource.Proxmox.SensorSetupOutdated = nil + if CanonicalResourceType(resource.Type) == ResourceTypeAgent { + resource.Proxmox.SensorSetupOutdated = sensorSetupVerdict(false) + } + } + if resource.ParentID != nil && physicalDiskWaitsOnSMARTTemperature(resource) { + waiting[*resource.ParentID] = struct{}{} + } + } + for parentID := range waiting { + node := rr.resources[parentID] + if node == nil || node.Proxmox == nil || CanonicalResourceType(node.Type) != ResourceTypeAgent { + continue + } + details := node.Proxmox.TemperatureDetails + node.Proxmox.SensorSetupOutdated = sensorSetupVerdict(details != nil && details.Available && details.LegacySensorsFormat) + } +} + +func sensorSetupVerdict(outdated bool) *bool { + return &outdated +} + +func physicalDiskWaitsOnSMARTTemperature(resource *Resource) bool { + if CanonicalResourceType(resource.Type) != ResourceTypePhysicalDisk || resource.PhysicalDisk == nil { + return false + } + disk := resource.PhysicalDisk + if _, ok := smartTemperatureDiskTypes[strings.ToLower(strings.TrimSpace(disk.DiskType))]; !ok { + return false + } + return !diskinventory.TemperatureCollected(disk.Temperature, disk.Collection) +} diff --git a/internal/unifiedresources/registry.go b/internal/unifiedresources/registry.go index 6c9a786cad..827d8dfc43 100644 --- a/internal/unifiedresources/registry.go +++ b/internal/unifiedresources/registry.go @@ -597,6 +597,7 @@ func (rr *ResourceRegistry) ingestSnapshot(snapshot models.StateSnapshot, thresh rr.refreshStoragePostureLocked() rr.refreshIncidentRollupsLocked() rr.buildChildCounts() + rr.refreshProxmoxSensorSetupLocked() rr.refreshCanonicalIdentitiesLocked() rr.markStaleLocked(time.Now().UTC(), thresholds) rr.invalidateViewsLocked() @@ -648,6 +649,7 @@ func (rr *ResourceRegistry) ingestRecords(source DataSource, records []IngestRec rr.refreshStoragePostureLocked() rr.refreshIncidentRollupsLocked() rr.buildChildCounts() + rr.refreshProxmoxSensorSetupLocked() rr.refreshCanonicalIdentitiesLocked() rr.invalidateViewsLocked() rr.mu.Unlock() @@ -843,6 +845,7 @@ func (rr *ResourceRegistry) ingestResources(resources []Resource, thresholds map rr.refreshStoragePostureLocked() rr.refreshIncidentRollupsLocked() rr.buildChildCounts() + rr.refreshProxmoxSensorSetupLocked() rr.refreshCanonicalIdentitiesLocked() rr.markStaleLocked(time.Now().UTC(), thresholds) rr.invalidateViewsLocked() diff --git a/internal/unifiedresources/registry_test.go b/internal/unifiedresources/registry_test.go index 7c98c19bca..f619ec2cb3 100644 --- a/internal/unifiedresources/registry_test.go +++ b/internal/unifiedresources/registry_test.go @@ -7144,3 +7144,248 @@ func TestPhysicalDiskRiskNeverJudgesTemperature(t *testing.T) { t.Fatalf("meta recompute risk = %+v, want none for a collected 72C", risk) } } + +// The Proxmox page's Overview lists nodes without their disk inventory, so the +// outdated sensor setup verdict has to arrive on the node itself. The registry +// derives it from the node's legacy-format temperature payload and the disks +// it parents, with the same reading rule every disk temperature consumer uses. +func TestProxmoxNodeSensorSetupOutdatedFromItsDisks(t *testing.T) { + now := time.Now() + legacy := &models.Temperature{Available: true, LegacySensorsFormat: true} + inventoryOnly := &diskinventory.CollectionStatus{ + Temperature: diskinventory.Unsupported("proxmox_disks", "Proxmox disk inventory does not expose temperature"), + } + node := func(name string, temperature *models.Temperature) models.Node { + return models.Node{ + ID: "homelab-" + name, + Name: name, + Instance: "homelab", + Status: "online", + LastSeen: now, + Temperature: temperature, + } + } + disk := func(nodeName, devPath, diskType string, temperature int, collection *diskinventory.CollectionStatus) models.PhysicalDisk { + return models.PhysicalDisk{ + ID: ProxmoxPhysicalDiskSourceID("homelab", nodeName, devPath, "", ""), + Node: nodeName, + Instance: "homelab", + DevPath: devPath, + Model: "Disk " + devPath, + Serial: "SERIAL-" + nodeName + devPath, + Type: diskType, + Health: "PASSED", + Wearout: -1, + Temperature: temperature, + Collection: collection, + LastChecked: now, + } + } + + cases := []struct { + name string + nodes []models.Node + disks []models.PhysicalDisk + want bool + }{ + { + name: "SATA disk without a temperature", + nodes: []models.Node{node("pve", legacy)}, + disks: []models.PhysicalDisk{disk("pve", "/dev/sda", "sata", 0, inventoryOnly)}, + want: true, + }, + { + // Proxmox's own disk inventory types these disks by form factor. + name: "Proxmox hdd and ssd inventory types", + nodes: []models.Node{node("pve", legacy)}, + disks: []models.PhysicalDisk{disk("pve", "/dev/sda", "hdd", 0, inventoryOnly), disk("pve", "/dev/sdb", "ssd", 0, inventoryOnly)}, + want: true, + }, + { + name: "SAS disk whose reading is retained, not current", + nodes: []models.Node{node("pve", legacy)}, + disks: []models.PhysicalDisk{disk("pve", "/dev/sda", "sas", 41, &diskinventory.CollectionStatus{ + Temperature: diskinventory.Unavailable("host_agent", "host agent stopped reporting"), + })}, + want: true, + }, + { + name: "SATA disk whose reading the source marks missing", + nodes: []models.Node{node("pve", legacy)}, + disks: []models.PhysicalDisk{disk("pve", "/dev/sda", "sata", 38, &diskinventory.CollectionStatus{ + Temperature: diskinventory.Missing("proxmox_node_smart", "temperature was not reported"), + })}, + want: true, + }, + { + name: "SATA disk with a current reading", + nodes: []models.Node{node("pve", legacy)}, + disks: []models.PhysicalDisk{disk("pve", "/dev/sda", "sata", 36, &diskinventory.CollectionStatus{ + Temperature: diskinventory.Available("proxmox_node_smart"), + })}, + want: false, + }, + { + name: "reading from a source that predates collection state", + nodes: []models.Node{node("pve", legacy)}, + disks: []models.PhysicalDisk{disk("pve", "/dev/sda", "sata", 36, nil)}, + want: false, + }, + { + // NVMe temperatures arrive through kernel hwmon even on a legacy setup. + name: "NVMe disk without a temperature", + nodes: []models.Node{node("pve", legacy)}, + disks: []models.PhysicalDisk{disk("pve", "/dev/nvme0n1", "nvme", 0, inventoryOnly)}, + want: false, + }, + { + name: "current setup payload", + nodes: []models.Node{node("pve", &models.Temperature{Available: true})}, + disks: []models.PhysicalDisk{disk("pve", "/dev/sda", "sata", 0, inventoryOnly)}, + want: false, + }, + { + name: "failed temperature collection", + nodes: []models.Node{node("pve", &models.Temperature{Available: false, LegacySensorsFormat: true})}, + disks: []models.PhysicalDisk{disk("pve", "/dev/sda", "sata", 0, inventoryOnly)}, + want: false, + }, + { + name: "waiting disk belongs to another node", + nodes: []models.Node{node("pve", legacy), node("pve2", &models.Temperature{Available: true})}, + disks: []models.PhysicalDisk{disk("pve2", "/dev/sda", "sata", 0, inventoryOnly)}, + want: false, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + registry := NewRegistry(nil) + registry.IngestSnapshot(models.StateSnapshot{Nodes: tc.nodes, PhysicalDisks: tc.disks}) + got := proxmoxNodeSensorSetupOutdated(t, registry.List(), "pve") + if got != tc.want { + t.Fatalf("pve sensorSetupOutdated = %v, want %v", got, tc.want) + } + }) + } +} + +// A linked host agent that reads the disk's SMART temperature itself answers +// what the legacy SSH setup cannot, so the node is not flagged; the verdict is +// derived on every ingest, so a later snapshot clears an earlier flag. +func TestProxmoxNodeSensorSetupOutdatedFollowsWhereTheReadingArrives(t *testing.T) { + now := time.Now() + nodes := []models.Node{{ + ID: "homelab-pve", + Name: "pve", + Instance: "homelab", + LinkedAgentID: "host-pve", + Status: "online", + LastSeen: now, + Temperature: &models.Temperature{Available: true, LegacySensorsFormat: true}, + }} + inventory := []models.PhysicalDisk{{ + ID: ProxmoxPhysicalDiskSourceID("homelab", "pve", "/dev/sda", "", ""), + Node: "pve", + Instance: "homelab", + DevPath: "/dev/sda", + Model: "WDC WD80EFAX", + Serial: "SERIAL-SDA", + Type: "hdd", + Health: "PASSED", + Wearout: -1, + Collection: &diskinventory.CollectionStatus{Temperature: diskinventory.Unsupported("proxmox_disks", "Proxmox disk inventory does not expose temperature")}, + LastChecked: now, + }} + agent := func(collection *diskinventory.CollectionStatus) []models.Host { + return []models.Host{{ + ID: "host-pve", + Hostname: "pve", + LinkedNodeID: "homelab-pve", + Status: "online", + LastSeen: now, + Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{{ + Device: "/dev/sda", + Model: "WDC WD80EFAX", + Serial: "SERIAL-SDA", + Type: "sata", + Temperature: 34, + Health: "PASSED", + Collection: collection, + }}}, + }} + } + + registry := NewRegistry(nil) + registry.IngestSnapshot(models.StateSnapshot{Nodes: nodes, PhysicalDisks: inventory}) + if !proxmoxNodeSensorSetupOutdated(t, registry.List(), "pve") { + t.Fatal("legacy node with an unread hdd: sensorSetupOutdated = false, want true") + } + + registry.IngestSnapshot(models.StateSnapshot{ + Nodes: nodes, + PhysicalDisks: inventory, + Hosts: agent(&diskinventory.CollectionStatus{Temperature: diskinventory.Available("host_agent")}), + }) + if proxmoxNodeSensorSetupOutdated(t, registry.List(), "pve") { + t.Fatal("linked agent reads the disk now: sensorSetupOutdated = true, want the earlier flag cleared") + } + // Clients merge a facet field by field, so the cleared verdict must be on + // the wire as false, not omitted. + assertProxmoxNodeSensorSetupJSON(t, registry.ListForPresentation(), "pve", `"sensorSetupOutdated":false`) + + registry.IngestSnapshot(models.StateSnapshot{ + Nodes: nodes, + PhysicalDisks: inventory, + Hosts: agent(&diskinventory.CollectionStatus{Temperature: diskinventory.Unavailable("host_agent", "host agent stopped reporting")}), + }) + presented := registry.ListForPresentation() + if !proxmoxNodeSensorSetupOutdated(t, presented, "pve") { + t.Fatal("agent reading retained, not current: sensorSetupOutdated = false, want true") + } + assertProxmoxNodeSensorSetupJSON(t, presented, "pve", `"sensorSetupOutdated":true`) +} + +func assertProxmoxNodeSensorSetupJSON(t *testing.T, resources []Resource, nodeName, want string) { + t.Helper() + for _, resource := range resources { + if resource.Proxmox == nil || resource.Proxmox.NodeName != nodeName || CanonicalResourceType(resource.Type) != ResourceTypeAgent { + continue + } + payload, err := json.Marshal(resource.Proxmox) + if err != nil { + t.Fatalf("marshal node proxmox facet: %v", err) + } + if !strings.Contains(string(payload), want) { + t.Fatalf("node proxmox payload = %s, want %s for list readers", payload, want) + } + return + } + t.Fatalf("node %q not presented", nodeName) +} + +func proxmoxNodeSensorSetupOutdated(t *testing.T, resources []Resource, nodeName string) bool { + t.Helper() + var found *Resource + for i := range resources { + resource := &resources[i] + if CanonicalResourceType(resource.Type) != ResourceTypeAgent || resource.Proxmox == nil || resource.Proxmox.NodeName != nodeName { + continue + } + if found != nil { + t.Fatalf("node %q presented twice: %s and %s", nodeName, found.ID, resource.ID) + } + found = resource + } + if found == nil { + t.Fatalf("node %q not found among %d resources", nodeName, len(resources)) + } + for _, resource := range resources { + if CanonicalResourceType(resource.Type) != ResourceTypeAgent && resource.Proxmox != nil && resource.Proxmox.SensorSetupOutdated != nil { + t.Fatalf("%s %s carries the node verdict", resource.Type, resource.ID) + } + } + if found.Proxmox.SensorSetupOutdated == nil { + t.Fatalf("node %q has no explicit sensor setup verdict", nodeName) + } + return *found.Proxmox.SensorSetupOutdated +} diff --git a/internal/unifiedresources/types.go b/internal/unifiedresources/types.go index fe8acee7c7..d54c5e831b 100644 --- a/internal/unifiedresources/types.go +++ b/internal/unifiedresources/types.go @@ -414,6 +414,14 @@ type ProxmoxData struct { MemoryCache int64 `json:"memoryCache,omitempty"` Memory *models.Memory `json:"memory,omitempty"` Lock string `json:"lock,omitempty"` // Proxmox lock state (e.g. "backup", "migrate", "snapshot") + // SensorSetupOutdated is derived by the registry, never ingested: the + // node's SSH temperature monitoring still runs the pre-rc.6 setup and a + // disk under it waits on a SMART temperature that setup cannot deliver + // (refreshProxmoxSensorSetupLocked). Every Proxmox node carries an + // explicit true or false, so clients that merge facets field by field + // replace a cleared verdict instead of keeping an earlier true; other + // resources omit it. + SensorSetupOutdated *bool `json:"sensorSetupOutdated,omitempty"` // Internal link hint to a host agent resource. LinkedAgentID string `json:"-"` } From a5b2a5a4c49bd2edc4cb3818f9adf8a62cda31b1 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 13:49:31 +0100 Subject: [PATCH 6/9] Refuse a seeded disk slot mapping when the disk was replaced ingestRecord reuses an existing source-key mapping before any identity matching, and for physical disks it checked only the type and parent scope. Proxmox keys a disk by its slot, so in a registry seeded from persisted unified resources a disk swapped into the slot mapped straight back to the previous disk's canonical resource. Where an agent report had been merged in, mergeInto also kept that disk's serial, WWN, temperature and failed health. A Proxmox disk observation now refuses the mapping when the two disks carry hardware identities naming different disks (diskinventory.HardwareIdentityConflict). Only fields reported on both sides count. Differing serials do not count for SAS (Proxmox may report the SAS address, #1595), or when the mapped disk was merged with an agent report and Proxmox did not read the drive's own serial, by the rule monitoring's SMART merge already uses. Collection status cannot decide that: a legacy agent's restored serial keeps Proxmox's status. The refused key falls through to the linked-disk join, findMatch and chooseNewID, but none of them may merge it back into the refused disk: an ID landing on it is replaced with the key's source-specific ID. Without that, a disk keyed by a USB bridge serial re-merged at once. No current caller replays Proxmox disk records over seeded mappings: the monitor rebuilds a fresh registry each poll. This closes the registry contract the seeded-mapping comment promises. --- .../internal/subsystems/unified-resources.md | 49 +++++- .../issue1595_collection_trust_test.go | 161 ++++++++++++++++++ internal/unifiedresources/registry.go | 75 +++++++- 3 files changed, 275 insertions(+), 10 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index 9d3644cb63..3c115496dd 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -6442,9 +6442,52 @@ conflict): the agent's row may be the slot's previous occupant, retained by a silent agent, and the join would hand the replacement that disk's serial, readings and canonical resource. A hardware identity match still joins on its own, and without a WWN on either side the path fallback stands. The seeded -source mapping reused after a restart is a separate path this does not cover. -`TestRegistrySASPathJoinRefusesContradictingWWN` covers the stale row and both -same-disk shapes; monitoring's SMART merge applies the matching guard first. +source mapping a rehydrated registry reuses applies the same refusal (next +section). `TestRegistrySASPathJoinRefusesContradictingWWN` covers the stale +row and both same-disk shapes; monitoring's SMART merge applies the matching +guard first. + +### Seeded slot mapping refuses a replaced disk + +`ingestRecord` honours a source key's existing mapping, such as one +`IngestResources` seeds from persisted unified resources, before any +identity matching. Proxmox keys a disk by its slot +(`ProxmoxPhysicalDiskSourceID`), so a disk swapped into the slot arrived +under the previous disk's key, and `mergeInto` kept that disk's canonical ID +and, where an agent report had been merged in, its serial, WWN, temperature +and failed health. A Proxmox disk observation now refuses the mapping when +the two carry hardware identities naming different disks +(`proxmoxDiskSlotHoldsAnotherDisk`: `HardwareIdentityConflict` after +`HardwareIdentityMatch`). Only fields reported on both sides count, so a +missing serial or WWN, including Proxmox's literal `unknown`, is no evidence. +Differing serials are no evidence when either side is SAS, because Proxmox +may report the SAS address there (#1595), or when the mapped disk was merged +with an agent report, whose serial `mergeInto` may have kept (collection +status does not reliably say which serial it holds), unless Proxmox read the +drive's own serial too, by the rule monitoring's +`hostAgentSMARTSerialComparable` applies: NVMe on both sides, or an agent +SATA disk under Proxmox vendor `ATA` that Proxmox does not type `usb`, not a +SCSI designator or a USB bridge's serial. A refused key falls through to the linked-disk join, +`findMatch` and `chooseNewID` like an unmapped one, except that none of them +may merge it into the refused disk: an ID that lands on it is replaced with +the key's source-specific ID. Agreement still wins, as it does in identity +matching, so drives that share a serial they do not own, such as a USB +bridge's, read as one disk: a second swap in the same enclosure joins the +first replacement, which reported the same bridge serial. That belongs to +identity matching, not this guard. The refused disk keeps its Proxmox +facet and is not updated, so until the registry is rebuilt both disks answer +to the slot's PVE alert reference, and canonical reference resolution treats +that reference as ambiguous rather than handing it to either. A later +rehydration may seed the slot key to the old disk again, and the refusal +repeats. No current caller ingests Proxmox disk records over a seeded +mapping: the monitor rebuilds a fresh registry each poll, and the resource +API, host-continuity and Patrol registries seeded from unified resources do +not replay Proxmox disk records. This closes the registry contract, not a +reported symptom. `TestRegistrySeededSlotMappingRefusesReplacedDisk` covers +SATA (with and without a new WWN), SAS, USB-enclosure and Proxmox-only +replacements, plus eight same-disk controls (including a SCSI designator +from an agent without collection status and a USB bridge serial under +Proxmox vendor `ATA`), each through two rehydrations. ### Drawer tab selection survives a transient snapshot change (#1723) diff --git a/internal/monitoring/issue1595_collection_trust_test.go b/internal/monitoring/issue1595_collection_trust_test.go index 7a52f71d8b..06e72b6a29 100644 --- a/internal/monitoring/issue1595_collection_trust_test.go +++ b/internal/monitoring/issue1595_collection_trust_test.go @@ -612,3 +612,164 @@ func TestRegistrySASPathJoinRefusesContradictingWWN(t *testing.T) { }) } } + +// A registry rehydrated from a persisted unified snapshot seeds each source +// key's mapping before the next poll arrives. Proxmox keys a disk by its slot, +// so a disk swapped into the slot arrives under the previous disk's key. The +// mapping is refused when the two carry hardware identities naming different +// disks, and nothing else may merge the replacement back into the old disk, +// so it gets its own canonical resource instead of the old disk's ID, serial, +// temperature and failed health. Identity missing on one side keeps the +// mapping, and so does a serial that may not be the drive's own (a SAS +// address, a SCSI designator, a USB bridge's serial) set against the agent's. +func TestRegistrySeededSlotMappingRefusesReplacedDisk(t *testing.T) { + const slotSourceID = "pve1-node1--dev-sdb" + // Each kind is the slot's previous disk as Proxmox and the agent reported + // it; the registry merged the two reports by WWN or by path, unless the + // case has Proxmox alone report it. + kinds := map[string]struct{ agentType, vendor, proxmoxType, proxmoxSerial string }{ + "sata": {agentType: "sata", vendor: "ATA", proxmoxType: "hdd", proxmoxSerial: "ZR5A0001"}, + "sas": {agentType: "sas", vendor: "SEAGATE", proxmoxType: "hdd", proxmoxSerial: "5000c500aaaa0003"}, + "usb": {agentType: "usb", vendor: "JMicron", proxmoxType: "usb", proxmoxSerial: "BRIDGE0001"}, + "scsi": {agentType: "sata", vendor: "SEAGATE", proxmoxType: "hdd", proxmoxSerial: "Z1Z0VPD0001"}, + "usb-ata": {agentType: "sata", vendor: "ATA", proxmoxType: "usb", proxmoxSerial: "BRIDGE0001"}, + } + for _, tc := range []struct { + name string + kind string + proxmoxFirst bool + proxmoxOnly bool + legacyAgent bool + serial string + wwn string + replacedDrive bool + }{ + {name: "replacement disk", kind: "sata", serial: "ZR5B0002", wwn: "0x5000c500bbbb0002", replacedDrive: true}, + {name: "replacement disk without a WWN", kind: "sata", serial: "ZR5B0002", wwn: "unknown", replacedDrive: true}, + {name: "same disk", kind: "sata", serial: "ZR5A0001", wwn: "0x5000c500aaaa0001"}, + {name: "same disk without a serial", kind: "sata", serial: "unknown", wwn: "0x5000c500aaaa0001"}, + {name: "SAS disk whose Proxmox serial is a SAS address", kind: "sas", serial: "5000c500aaaa0003", wwn: "0x5000c500aaaa0001"}, + {name: "SAS disk without a Proxmox WWN", kind: "sas", serial: "5000c500aaaa0003", wwn: "unknown"}, + {name: "USB disk whose Proxmox serial is the bridge's", kind: "usb", serial: "BRIDGE0001", wwn: "unknown"}, + {name: "USB bridge serial under Proxmox vendor ATA", kind: "usb-ata", serial: "BRIDGE0001", wwn: "unknown"}, + {name: "disk whose Proxmox serial is a SCSI designator", kind: "scsi", serial: "Z1Z0VPD0001", wwn: "unknown"}, + {name: "SAS replacement disk", kind: "sas", serial: "5000c500bbbb0003", wwn: "0x5000c500bbbb0002", replacedDrive: true}, + {name: "drive swapped in the same USB enclosure", kind: "usb", proxmoxFirst: true, + serial: "BRIDGE0001", wwn: "0x5000c500bbbb0002", replacedDrive: true}, + {name: "SCSI designator from an agent without collection status", kind: "scsi", legacyAgent: true, + serial: "Z1Z0VPD0001", wwn: "unknown"}, + {name: "replacement Proxmox alone reports, without a WWN", kind: "scsi", proxmoxOnly: true, + serial: "Z1Z0VPD0002", wwn: "unknown", replacedDrive: true}, + } { + t.Run(tc.name, func(t *testing.T) { + kind := kinds[tc.kind] + now := time.Now() + node := models.Node{ + ID: "pve1-node1", Name: "node1", Instance: "pve1", Status: "online", + LastSeen: now, LinkedAgentID: "agent-1", + } + host := models.Host{ + ID: "agent-1", Hostname: "node1", LinkedNodeID: "pve1-node1", Status: "online", LastSeen: now, + Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{{ + Device: "/dev/sdb", Serial: "ZR5A0001", WWN: "5-c50-aaaa0001", Type: kind.agentType, + Health: "FAILED", Temperature: 55, + Collection: &diskinventory.CollectionStatus{ + Serial: diskinventory.Available("smartctl"), + Temperature: diskinventory.Available("smartctl"), + }, + }}}, + } + // As monitor_pve builds the slot's row from disks/list. + slotDisk := func(serial, wwn, health string) models.PhysicalDisk { + disk := models.PhysicalDisk{ + ID: slotSourceID, Node: "node1", Instance: "pve1", DevPath: "/dev/sdb", Vendor: kind.vendor, + Serial: proxmoxReportedDiskIdentity(serial), WWN: proxmoxReportedDiskIdentity(wwn), + Type: kind.proxmoxType, Health: health, Wearout: -1, LastChecked: now, + Collection: &diskinventory.CollectionStatus{ + Serial: diskinventory.Missing("proxmox_disks", "disk serial was not reported"), + }, + } + if disk.Serial != "" { + disk.Collection.Serial = diskinventory.Available("proxmox_disks") + } + return disk + } + previousDisk := slotDisk(kind.proxmoxSerial, "0x5000c500aaaa0001", "FAILED") + previous := unifiedresources.NewRegistry(nil) + if tc.proxmoxFirst { + previous.IngestSnapshot(models.StateSnapshot{ + Nodes: []models.Node{node}, PhysicalDisks: []models.PhysicalDisk{previousDisk}, + }) + } + hosts := []models.Host{host} + if tc.proxmoxOnly { + hosts = nil + } + if tc.legacyAgent { + host.Sensors.SMART[0].Collection = nil + } + previous.IngestSnapshot(models.StateSnapshot{ + Nodes: []models.Node{node}, Hosts: hosts, PhysicalDisks: []models.PhysicalDisk{previousDisk}, + }) + persisted := previous.ListByType(unifiedresources.ResourceTypePhysicalDisk) + if len(persisted) != 1 || persisted[0].PhysicalDisk == nil || + !hasIssue1595Source(persisted[0].Sources, unifiedresources.SourceProxmox) || + hasIssue1595Source(persisted[0].Sources, unifiedresources.SourceAgent) == tc.proxmoxOnly { + t.Fatalf("want one disk merged from Proxmox and the agent (Proxmox alone: %v), got %+v", tc.proxmoxOnly, persisted) + } + oldID, oldDisk := persisted[0].ID, *persisted[0].PhysicalDisk + + // Restart: rehydrate from the persisted resources, then poll + // Proxmox before the agent has reported again. A second restart + // rehydrates from the result, where both disks may claim the slot. + current := slotDisk(tc.serial, tc.wwn, "PASSED") + seed := previous.List() + slotID := "" + for restart := 1; restart <= 2; restart++ { + rehydrated := unifiedresources.NewRegistry(nil) + rehydrated.IngestResources(seed) + rehydrated.IngestSnapshot(models.StateSnapshot{ + Nodes: []models.Node{node}, PhysicalDisks: []models.PhysicalDisk{current}, + }) + seed = rehydrated.List() + + var slot *unifiedresources.Resource + disks := rehydrated.ListByType(unifiedresources.ResourceTypePhysicalDisk) + for index := range disks { + for _, target := range rehydrated.SourceTargets(disks[index].ID) { + if target.Source == unifiedresources.SourceProxmox && target.SourceID == slotSourceID { + slot = &disks[index] + } + } + } + if slot == nil || slot.PhysicalDisk == nil { + t.Fatalf("restart %d: no disk holds the Proxmox slot key among %d disks", restart, len(disks)) + } + if slotID != "" && slot.ID != slotID { + t.Fatalf("restart %d: slot moved from %s to %s", restart, slotID, slot.ID) + } + slotID = slot.ID + if !tc.replacedDrive { + if slot.ID != oldID || len(disks) != 1 { + t.Fatalf("restart %d: slot disk = %s with %d disks, want the persisted disk %s alone", + restart, slot.ID, len(disks), oldID) + } + continue + } + if slot.ID == oldID || len(disks) != 2 { + t.Fatalf("restart %d: replacement took the persisted disk's canonical ID %s (%d disks)", restart, oldID, len(disks)) + } + if slot.PhysicalDisk.Serial != current.Serial || slot.PhysicalDisk.WWN != current.WWN || + slot.PhysicalDisk.Temperature != 0 || slot.PhysicalDisk.Health != "PASSED" { + t.Fatalf("restart %d: replacement took the persisted disk's identity or readings: %+v", restart, slot.PhysicalDisk) + } + for _, disk := range disks { + if disk.ID == oldID && (disk.PhysicalDisk == nil || + disk.PhysicalDisk.Serial != oldDisk.Serial || disk.PhysicalDisk.WWN != oldDisk.WWN) { + t.Fatalf("restart %d: persisted disk was rewritten: %+v", restart, disk.PhysicalDisk) + } + } + } + }) + } +} diff --git a/internal/unifiedresources/registry.go b/internal/unifiedresources/registry.go index 7d59ac78b4..12eed901a2 100644 --- a/internal/unifiedresources/registry.go +++ b/internal/unifiedresources/registry.go @@ -2882,22 +2882,30 @@ func (rr *ResourceRegistry) ingestRecord(source DataSource, sourceID string, res // Rehydrated registries seed exact source mappings from the persisted // unified snapshot. Honor that durable mapping before attempting weaker // identity correlation, but fail closed if a colliding source key belongs - // to a different physical-disk parent. + // to a different physical-disk parent. A Proxmox disk key names a slot, so + // it is also refused when the slot now holds a different disk, and no + // later step may merge the observation into the refused disk either. + refusedDiskID := "" if mappedID := rr.bySource[source][sourceID]; mappedID != "" { if existing := rr.resources[mappedID]; existing != nil && existing.Type == resource.Type && (resource.Type != ResourceTypePhysicalDisk || physicalDiskMatchScopeCompatible(existing, &resource)) { - if onlyMissing { - return "" + if source == SourceProxmox && resource.Type == ResourceTypePhysicalDisk && + proxmoxDiskSlotHoldsAnotherDisk(existing, &resource) { + refusedDiskID = existing.ID + } else { + if onlyMissing { + return "" + } + rr.mergeInto(existing, resource, source, sourceID) + return existing.ID } - rr.mergeInto(existing, resource, source, sourceID) - return existing.ID } } // Linked resources must be mutually linked to avoid one-sided/ambiguous auto-merges. - if linked := rr.resolveLinkedResource(source, sourceID, resource); linked != "" { + if linked := rr.resolveLinkedResource(source, sourceID, resource); linked != "" && linked != refusedDiskID { existing := rr.resources[linked] if existing != nil { if onlyMissing { @@ -2912,7 +2920,7 @@ func (rr *ResourceRegistry) ingestRecord(source DataSource, sourceID string, res candidateID := rr.sourceSpecificID(resource.Type, source, sourceID) if resource.Type == ResourceTypeAgent || resource.Type == ResourceTypePhysicalDisk { - if match, excluded := rr.findMatch(resource, candidateID); match != nil { + if match, excluded := rr.findMatch(resource, candidateID); match != nil && match.ResourceB != refusedDiskID { existing := rr.resources[match.ResourceB] if existing != nil { if onlyMissing { @@ -2937,6 +2945,9 @@ func (rr *ResourceRegistry) ingestRecord(source DataSource, sourceID string, res resource.ID = rr.chooseNewID(resource.Type, identity, source, sourceID) normalizeResourceRelationships(&resource) + if resource.ID == refusedDiskID { + resource.ID = candidateID + } if existing := rr.resources[resource.ID]; existing != nil { if onlyMissing { return "" @@ -3186,6 +3197,56 @@ func physicalDiskMatchScopeCompatible(existing, incoming *Resource) bool { return identitiesShareHostname(existing.Identity, incoming.Identity) } +// proxmoxDiskSlotHoldsAnotherDisk reports whether a Proxmox disk observed +// under a mapped slot key (ProxmoxPhysicalDiskSourceID) names a different disk +// than the resource the key maps to: the slot's disk was swapped, and merging +// would keep the previous disk's canonical ID and, where an agent report was +// merged in, its serial, WWN and readings. Only hardware identity reported on +// both sides counts, and differing serials only where +// physicalDiskMappedSerialsComparable says both are the drive's own. +func proxmoxDiskSlotHoldsAnotherDisk(existing, incoming *Resource) bool { + if existing.PhysicalDisk == nil || incoming.PhysicalDisk == nil { + return false + } + existingSerial, existingWWN := existing.PhysicalDisk.Serial, existing.PhysicalDisk.WWN + incomingSerial, incomingWWN := incoming.PhysicalDisk.Serial, incoming.PhysicalDisk.WWN + if diskinventory.HardwareIdentityMatch(existingSerial, existingWWN, incomingSerial, incomingWWN) { + return false + } + if !physicalDiskMappedSerialsComparable(existing, incoming) { + existingSerial, incomingSerial = "", "" + } + return diskinventory.HardwareIdentityConflict(existingSerial, existingWWN, incomingSerial, incomingWWN) +} + +// physicalDiskMappedSerialsComparable reports whether differing serials on a +// mapped disk and a Proxmox observation under its slot key name different +// disks. Proxmox may report a SAS disk's transport address as its serial +// (#1595). A disk Proxmox alone reports carries Proxmox's serial, compared as +// is. A disk merged with an agent report may carry the agent's serial instead +// (mergeInto restores it, and collection status does not reliably say which), +// so its serial is compared only where Proxmox read the drive's own serial +// too, as monitoring's hostAgentSMARTSerialComparable decides: an NVMe +// controller serial, or an ATA drive's IDENTIFY serial, not a SCSI designator +// or a USB bridge's serial. +func physicalDiskMappedSerialsComparable(existing, incoming *Resource) bool { + switch { + case physicalDiskTypeIs(existing, "sas") || physicalDiskTypeIs(incoming, "sas"): + return false + case !hasDataSource(existing.Sources, SourceAgent): + return true + case physicalDiskTypeIs(incoming, "nvme"): + return physicalDiskTypeIs(existing, "nvme") + case physicalDiskTypeIs(incoming, "usb"): + return false + } + return physicalDiskTypeIs(existing, "sata") && strings.EqualFold(strings.TrimSpace(incoming.PhysicalDisk.Vendor), "ATA") +} + +func physicalDiskTypeIs(disk *Resource, diskType string) bool { + return strings.EqualFold(strings.TrimSpace(disk.PhysicalDisk.DiskType), diskType) +} + func (rr *ResourceRegistry) resolveLinkedResource(source DataSource, sourceID string, resource Resource) string { if resource.Type == ResourceTypePhysicalDisk && (source == SourceAgent || source == SourceProxmox) { From c9d4cdebf04194662d5b367a5cda6cc253533008 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 14:06:37 +0100 Subject: [PATCH 7/9] Read a silent in-guest agent's guest drawer readings as last known MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Pulse agent linked into a VM keeps its last report on the VM resource after it stops reporting: the registry copies the agent's whole facet, agent.stale included, onto the guest. The guest drawer never read that flag, so a quiet agent's RAID kept its live colours and "Rebuild: 42% · 118M/sec", its network rows and version read as current, and nothing said the agent had gone. - useWorkloads maps agent.stale onto the row as agentStale, and sets disksFromAgent when Proxmox reported no guest filesystems and the agent's own disks filled in. - The guest drawer passes the stopped-reporting reason to RaidCard, puts a status line over muted network rows, marks the Pulse Agent row as stopped reporting, and reads agent-supplied filesystem rows as last known. - Agent-supplied filesystem rows follow the agent's freshness instead of the Proxmox guest-read reason. A VM with a live linked agent but no qemu-guest-agent showed the agent's filesystems as "Usage unavailable" with advice to install qemu-guest-agent. A stopped VM keeps the Proxmox reason, and Proxmox's own guest filesystems keep their read state. The workload table's disk cell, the drawer History, and the agent's OS, addresses and physical disk health still carry no stale cue. --- .../subsystems/performance-and-scalability.md | 18 +++ .../d4fbbd15ad939e02.json | 47 ++++++ .../components/Workloads/GuestDrawer.test.tsx | 151 ++++++++++++++++++ .../Workloads/GuestDrawerOverview.tsx | 71 ++++++-- .../src/hooks/__tests__/useWorkloads.test.ts | 48 ++++++ frontend-modern/src/hooks/useWorkloads.ts | 5 + frontend-modern/src/types/workloads.ts | 10 ++ 7 files changed, 337 insertions(+), 13 deletions(-) create mode 100644 frontend-modern/browser-verification/d4fbbd15ad939e02.json diff --git a/docs/release-control/v6/internal/subsystems/performance-and-scalability.md b/docs/release-control/v6/internal/subsystems/performance-and-scalability.md index 55a9ba06fa..b339107a3e 100644 --- a/docs/release-control/v6/internal/subsystems/performance-and-scalability.md +++ b/docs/release-control/v6/internal/subsystems/performance-and-scalability.md @@ -2732,6 +2732,24 @@ must remain distinguishable without hover. Workloads supplies this row metadata, not local table CSS or a duplicate renderer; non-filesystem rows keep the compact default. Verification covers the opt-in at this composition boundary and actual long-path wrapping in the shared renderer's browser proof. +A guest's linked Pulse Agent can stop reporting while Proxmox keeps polling the +VM. `useWorkloads` copies `resource.agent.stale` onto the row as `agentStale`, +and sets `disksFromAgent` when Proxmox reported no guest filesystems and the +agent's own `disks` filled in. When that agent is stale, `GuestDrawerOverview.tsx` +reads the Guest RAID card as last known through the shared `RaidCard` +`lastKnownReason` prop with `HOST_AGENT_STOPPED_REPORTING_REASON`, puts a status +line over muted Network rows, reads agent-supplied Filesystems rows through the +existing `last-known` disk read state, and marks the Pulse Agent row as stopped +reporting. Agent-supplied filesystem rows follow the agent's freshness rather +than the Proxmox `diskStatusReason`, which describes Proxmox's own guest reads, +so a live agent's rows keep their usage and bars. Those rows read as +unavailable once the guest is stopped, whether its status or the Proxmox +`vm-stopped` reason says so, and Proxmox's own guest filesystems keep their +Proxmox read state. The two flags ride the existing row mapping and +add no query or poll; `GuestDrawer.test.tsx` and `useWorkloads.test.ts` pin +both sources. The workload table's disk cell and the drawer History still read +the aggregate disk metric and the Proxmox reason, and the agent's OS, addresses +and physical disk health carry no stale cue yet. Guest, node, and Docker-host drawer headers follow the same frontend-primitives dependency boundary for collapse: Workloads owns which inline row is selected and the close handler, while diff --git a/frontend-modern/browser-verification/d4fbbd15ad939e02.json b/frontend-modern/browser-verification/d4fbbd15ad939e02.json new file mode 100644 index 0000000000..28d656c133 --- /dev/null +++ b/frontend-modern/browser-verification/d4fbbd15ad939e02.json @@ -0,0 +1,47 @@ +{ + "version": 1, + "base_sha": "43a80dafd91e92d3079a039cdb08929281812eef", + "verified_at": "2026-10-07T13:06:25Z", + "result": "passed", + "changed_paths": [ + "frontend-modern/src/components/Workloads/GuestDrawerOverview.tsx", + "frontend-modern/src/hooks/useWorkloads.ts", + "frontend-modern/src/types/workloads.ts" + ], + "content_sha256": { + "frontend-modern/src/components/Workloads/GuestDrawerOverview.tsx": "f984547722c304cd3c80610a3e5ef743b3e6ad268493016ddc649c858e54e6de", + "frontend-modern/src/hooks/useWorkloads.ts": "1439f5bc51ad5e362eb21b65b9d95b870f84cdae823770f4ae41dd848905703d", + "frontend-modern/src/types/workloads.ts": "4eebb6b801a056900e7fc309679cdca891b48bdd2672e40dbb58eaffce4ca0c6" + }, + "routes": [ + "/proxmox?q=analytics-batch-310", + "/proxmox?q=analytics-batch-118", + "/proxmox?q=analytics-batch-58", + "/proxmox?q=analytics-batch-262" + ], + "viewports": [ + { + "width": 1440, + "height": 900 + }, + { + "width": 390, + "height": 844 + } + ], + "states": [ + "Private mock stack built from origin/main 43a80dafd9 plus this change, with a local-only go build -overlay of internal/mock/generator.go: host agents apollo-114 and meridian-731 held past their reporting lease (offline, LastSeen frozen 14 minutes back, readings retained); all four standalone agents seeded with md0 raid1 clean and md1 raid5 'clean, degraded, recovering' at 42.4% / 118M/sec (sdd1 faulty, sde1 spare rebuilding); analytics-batch-310 and analytics-batch-58 forced to report no Proxmox guest filesystems with diskStatusReason agent-not-running", + "Agents linked into VMs through the real POST /api/resources/{vm}/link: apollo-114 -> analytics-batch-118, meridian-731 -> analytics-batch-310 (both silent), spectrum-709 -> analytics-batch-262 and meridian-848 -> analytics-batch-58 (both live). /api/resources serves agent.stale true with the agent's raid, disks and networkInterfaces on the two silent VMs and no stale flag on the two live ones", + "Before this change (same stack, PR 2652 head, real browser run): silent VMs showed Guest RAID with emerald 'clean', amber 'clean, degraded, recovering', 'Rebuild: 42% \u00b7 118M/sec' and coloured member badges, network rows and 'Pulse 0.2.0-alpha' as current; the live analytics-batch-58 showed its agent filesystems as 'Usage unavailable \u00b7 ?/328 GB' under 'Guest agent not running. Install and start qemu-guest-agent in the VM.'", + "After, silent analytics-batch-310 (agent filesystems) at 1440 and 390: Pulse Agent row 'Pulse 0.2.0-alpha \u00b7 stopped reporting' in amber with title 'Pulse Agent 0.2.0-alpha has stopped reporting. Its readings here are its last report.'; Filesystems status 'Using last known disk stats. The Pulse Agent in this guest stopped reporting.' with 'Last known 75%' / 'Last known 42%' rows and no bars; Network status 'Using last known interface details. The Pulse Agent in this guest stopped reporting.' over a muted eth0 row; both RAID arrays data-raid-reading=last-known, '(last known)' states in muted text titled 'Last known reading, not current: host agent stopped reporting', 'Rebuild was at 42%', neutral badges 'sdd1 \u00b7 faulty' and 'sde1 \u00b7 spare rebuilding'", + "After, silent analytics-batch-118 with Proxmox guest filesystems (earlier run of the same change): RAID, network and agent row read as above while the Proxmox filesystem rows kept their usage and bars", + "After, live analytics-batch-58 (agent filesystems, Proxmox agent-not-running): '/ 65%' and '/data 28%' with progress bars, no status line, no qemu-guest-agent advice; RAID current with emerald/amber states and 'Rebuild: 42% \u00b7 118M/sec'", + "After, live control analytics-batch-262 (Proxmox filesystems): unchanged at both widths", + "No horizontal page overflow at either width; no page errors. Stopped guests were not exercised in the browser: their agent filesystems read 'Usage unavailable' under the stopped-VM message whether the guest status or the Proxmox vm-stopped reason says stopped, and the stale flip on an open drawer, are covered by GuestDrawer.test.tsx. The final stopped-status gate changes only stopped guests, so the running-guest states above were observed on this code path" + ], + "interactions": [ + "Opened each guest's inline drawer from the Proxmox workloads table filtered by name (name cell click at 1440, first visible cell tap at 390)", + "Scrolled the phone drawer to the Network section and Guest RAID card and captured both", + "Read computed text colour, titles and data-raid-reading of every Filesystems, Network, System and RAID row via Playwright" + ] +} diff --git a/frontend-modern/src/components/Workloads/GuestDrawer.test.tsx b/frontend-modern/src/components/Workloads/GuestDrawer.test.tsx index 5f094a8215..2e4f080e38 100644 --- a/frontend-modern/src/components/Workloads/GuestDrawer.test.tsx +++ b/frontend-modern/src/components/Workloads/GuestDrawer.test.tsx @@ -1602,6 +1602,157 @@ describe('GuestDrawer', () => { }); }); + // ── In-guest Pulse Agent readings ── + + describe('In-guest Pulse Agent readings', () => { + // Proxmox read no guest filesystems, so these rows came from the agent. + const makeAgentGuest = (overrides: Partial = {}) => + makeGuest({ + agentVersion: '6.4.5', + agentKind: 'pulse', + disksFromAgent: true, + diskStatusReason: 'agent-not-running', + disks: [{ total: 100, used: 75, free: 25, usage: 75, mountpoint: '/' }], + networkInterfaces: [{ name: 'eth0', addresses: ['192.168.1.5'], rxBytes: 1024 }], + agentRaid: [ + { + device: '/dev/md1', + name: 'data', + level: 'raid5', + state: 'clean, degraded, recovering', + totalDevices: 3, + activeDevices: 2, + workingDevices: 3, + failedDevices: 1, + spareDevices: 0, + devices: [{ device: 'sdd1', state: 'faulty', slot: 1 }], + rebuildPercent: 42, + rebuildSpeed: '118M/sec', + }, + ], + ...overrides, + }); + + it("reads a silent in-guest agent's RAID, network and filesystems as last known", () => { + render(() => ); + + expect(technicalDetails().getByText('Pulse 6.4.5 · stopped reporting')).toHaveAttribute( + 'title', + 'Pulse Agent 6.4.5 has stopped reporting. Its readings here are its last report.', + ); + expect( + technicalDetails().getByText( + 'Using last known disk stats. The Pulse Agent in this guest stopped reporting.', + ), + ).toBeInTheDocument(); + expect(technicalDetails().getByText(/^Last known 75%/)).toBeInTheDocument(); + expect( + technicalDetails().queryByRole('progressbar', { name: 'Filesystem / utilization' }), + ).toBeNull(); + expect( + technicalDetails().getByText( + 'Using last known interface details. The Pulse Agent in this guest stopped reporting.', + ), + ).toBeInTheDocument(); + expect( + technicalDetails() + .getByText(/^192\.168\.1\.5/) + .closest('td'), + ).toHaveClass('text-muted'); + // Proxmox's guest-read reason does not describe rows the agent supplied. + expect(technicalDetails().queryByText(/qemu-guest-agent/)).toBeNull(); + + const array = screen.getByTestId('raid-card-array'); + expect(array).toHaveAttribute('data-raid-reading', 'last-known'); + expect(within(array).getByTestId('raid-card-state')).toHaveAttribute( + 'title', + 'Last known reading, not current: host agent stopped reporting', + ); + expect(within(array).getByTestId('raid-card-rebuild')).toHaveTextContent( + 'Rebuild was at 42%', + ); + }); + + it('keeps a live in-guest agent current when Proxmox has no guest reads', () => { + render(() => ); + + expect(technicalDetails().getByText('Pulse 6.4.5')).toBeInTheDocument(); + expect( + technicalDetails().getByRole('progressbar', { name: 'Filesystem / utilization' }), + ).toHaveAttribute('aria-valuenow', '75'); + expect(technicalDetails().queryByText(/Last known|qemu-guest-agent/)).toBeNull(); + expect(screen.getByTestId('raid-card-array')).toHaveAttribute('data-raid-reading', 'current'); + expect(screen.getByTestId('raid-card-rebuild')).toHaveTextContent('Rebuild: 42% · 118M/sec'); + }); + + it("keeps Proxmox's own guest filesystems current under a silent agent", () => { + render(() => ( + + )); + + expect( + technicalDetails().getByRole('progressbar', { name: 'Filesystem / utilization' }), + ).toHaveAttribute('aria-valuenow', '75'); + expect(technicalDetails().queryByText(/Using last known disk stats/)).toBeNull(); + expect(screen.getByTestId('raid-card-array')).toHaveAttribute( + 'data-raid-reading', + 'last-known', + ); + }); + + // Proxmox gives vm-stopped only for a guest with allocated disk, so the + // guest's own stopped status must hold the rows too. + it.each([ + ['with the Proxmox reason', 'vm-stopped'], + ['without a Proxmox reason', undefined], + ])( + "reads a stopped guest's agent filesystems as unavailable before the lease lapses, %s", + (_label, diskStatusReason) => { + render(() => ( + + )); + + expect( + technicalDetails().getByText( + 'Guest filesystem stats unavailable while the VM is stopped.', + ), + ).toBeInTheDocument(); + expect(technicalDetails().getByText(/^Usage unavailable/)).toBeInTheDocument(); + expect( + technicalDetails().queryByRole('progressbar', { name: 'Filesystem / utilization' }), + ).toBeNull(); + }, + ); + + it('follows the agent between silent and reporting while the drawer stays open', () => { + const [guest, setGuest] = createSignal(makeAgentGuest()); + render(() => ); + expect(screen.getByTestId('raid-card-array')).toHaveAttribute('data-raid-reading', 'current'); + + setGuest({ ...guest(), agentStale: true }); + expect(screen.getByTestId('raid-card-array')).toHaveAttribute( + 'data-raid-reading', + 'last-known', + ); + expect(technicalDetails().getByText('Pulse 6.4.5 · stopped reporting')).toBeInTheDocument(); + expect(technicalDetails().getByText(/^Last known 75%/)).toBeInTheDocument(); + + setGuest({ ...guest(), agentStale: undefined }); + expect(screen.getByTestId('raid-card-array')).toHaveAttribute('data-raid-reading', 'current'); + expect(technicalDetails().queryByText(/stopped reporting|Last known/)).toBeNull(); + }); + }); + // ── WebInterfaceUrlField ── describe('WebInterfaceUrlField', () => { diff --git a/frontend-modern/src/components/Workloads/GuestDrawerOverview.tsx b/frontend-modern/src/components/Workloads/GuestDrawerOverview.tsx index 072c4c3e4a..7218755137 100644 --- a/frontend-modern/src/components/Workloads/GuestDrawerOverview.tsx +++ b/frontend-modern/src/components/Workloads/GuestDrawerOverview.tsx @@ -10,6 +10,7 @@ import { TechnicalDetailsSection } from '@/components/shared/TechnicalDetailsDis import { DrawerAttentionSection } from '@/components/shared/DrawerAttentionSection'; import type { Alert } from '@/types/api'; import { AvailabilityProbeStatusCards } from '@/components/Infrastructure/AvailabilityProbeStatusCard'; +import { HOST_AGENT_STOPPED_REPORTING_REASON } from '@/components/Infrastructure/resourceDetailMappers'; import type { DiscoveryIdentifiedSummary } from '@/utils/discoveryPresentation'; import { formatBytes } from '@/utils/format'; import type { MetricDisplayThresholds } from '@/utils/metricThresholds'; @@ -79,6 +80,10 @@ export function GuestDrawerOverview(props: GuestDrawerOverviewProps) { ageClass: props.backupPresentation?.ageClass, }); const guestReadPresentation = () => getGuestDrawerGuestReadPresentation(props.guest); + // A linked Pulse agent past its reporting lease leaves its last report on + // the guest, so its RAID, network and filesystem rows read as last known. + const agentLastKnownReason = () => + props.guest.agentStale ? HOST_AGENT_STOPPED_REPORTING_REASON : undefined; const coverageLabel = () => { if (props.hasWorkloadActionAgent) return WORKLOAD_ACTION_AGENT_LABEL; if (props.hasAgentInfo) return `${props.agentHeading} observed`; @@ -147,9 +152,22 @@ export function GuestDrawerOverview(props: GuestDrawerOverviewProps) { props.guest.vmware?.datacenterName || props.guest.vmware?.clusterName, ); - const diskReason = isGuestDrawerVM(props.guest) ? props.guest.diskStatusReason : undefined; - const diskReadState = - props.guest.telemetryAvailability?.disk === false + // Rows the linked Pulse agent supplied follow that agent's freshness, not + // the Proxmox reason for its own guest reads. A stopped guest has no + // current filesystems from either source, with or without the Proxmox + // vm-stopped reason, which it gives only for a guest with allocated disk. + const vmDiskReason = isGuestDrawerVM(props.guest) ? props.guest.diskStatusReason : undefined; + const agentDisks = props.guest.disksFromAgent === true; + const agentDisksStopped = + agentDisks && (props.guest.status === 'stopped' || vmDiskReason === 'vm-stopped'); + const diskReason = agentDisks ? undefined : vmDiskReason; + const diskReadState = agentDisks + ? agentDisksStopped + ? 'unavailable' + : agentLastKnownReason() + ? 'last-known' + : 'current' + : props.guest.telemetryAvailability?.disk === false ? 'unavailable' : diskReason ? diskReason.startsWith('prev-') @@ -159,11 +177,19 @@ export function GuestDrawerOverview(props: GuestDrawerOverviewProps) { const diskRows = (props.guest.disks ?? []).map((disk, index) => buildWorkloadsDiskPresentation(disk, index, props.diskThresholds, diskReadState), ); - const diskStatusMessage = diskReason - ? getWorkloadGuestDiskStatusMessage(diskReason) - : diskReadState === 'unavailable' - ? 'Filesystem usage is unavailable.' - : null; + const diskStatusMessage = agentDisks + ? agentDisksStopped + ? isGuestDrawerVM(props.guest) + ? getWorkloadGuestDiskStatusMessage('vm-stopped') + : 'Filesystem usage is unavailable.' + : agentLastKnownReason() + ? 'Using last known disk stats. The Pulse Agent in this guest stopped reporting.' + : null + : diskReason + ? getWorkloadGuestDiskStatusMessage(diskReason) + : diskReadState === 'unavailable' + ? 'Filesystem usage is unavailable.' + : null; return compactDetailSections([ discovery @@ -186,7 +212,13 @@ export function GuestDrawerOverview(props: GuestDrawerOverviewProps) { rows: compactDetailRows([ props.guest.cpus ? makeDetailRow('CPUs', `${props.guest.cpus}`) : null, props.hasAgentInfo - ? makeDetailRow(props.agentHeading, props.agentLabel, { title: props.agentTitle }) + ? agentLastKnownReason() + ? makeDetailRow(props.agentHeading, `${props.agentLabel} · stopped reporting`, { + title: `${props.agentTitle} has stopped reporting. Its readings here are its last report.`, + tone: 'warning', + wrap: true, + }) + : makeDetailRow(props.agentHeading, props.agentLabel, { title: props.agentTitle }) : null, ]), }, @@ -272,8 +304,16 @@ export function GuestDrawerOverview(props: GuestDrawerOverviewProps) { props.hasNetworkInterfaces ? { label: 'Network', - rows: compactDetailRows( - props.networkInterfaces.slice(0, 4).map((iface, index) => { + rows: compactDetailRows([ + // Only the linked Pulse agent reports guest interfaces. + makeDetailRow( + 'Status', + agentLastKnownReason() + ? 'Using last known interface details. The Pulse Agent in this guest stopped reporting.' + : null, + { layout: 'stacked', wrap: true }, + ), + ...props.networkInterfaces.slice(0, 4).map((iface, index) => { const addresses = iface.addresses ?? []; const hasTraffic = (iface.rxBytes ?? 0) > 0 || (iface.txBytes ?? 0) > 0; const detail = [ @@ -291,11 +331,12 @@ export function GuestDrawerOverview(props: GuestDrawerOverviewProps) { iface.name || `Interface ${index + 1}`, detail || 'No details', { + tone: agentLastKnownReason() ? 'muted' : undefined, wrap: true, }, ); }), - ), + ]), } : null, ]); @@ -338,7 +379,11 @@ export function GuestDrawerOverview(props: GuestDrawerOverviewProps) { - +
diff --git a/frontend-modern/src/hooks/__tests__/useWorkloads.test.ts b/frontend-modern/src/hooks/__tests__/useWorkloads.test.ts index 8a19338644..1549c6c930 100644 --- a/frontend-modern/src/hooks/__tests__/useWorkloads.test.ts +++ b/frontend-modern/src/hooks/__tests__/useWorkloads.test.ts @@ -142,6 +142,54 @@ describe('useWorkloads', () => { } }); + it('marks a silent linked agent and the guest filesystems it supplied', async () => { + const disk = (mountpoint: string) => ({ + mountpoint, + type: 'ext4', + total: 100, + used: 40, + free: 60, + usage: 40, + }); + const [snapshot] = createSignal([ + // Proxmox read no guest filesystems, so the agent's own report fills in. + { + ...sampleResource, + id: 'vm-agent-disks', + vmid: 102, + sources: ['proxmox', 'agent'], + agent: { agentVersion: '6.4.5', stale: true, disks: [disk('/agent')] }, + }, + // Proxmox guest filesystems win over a live agent's. + { + ...sampleResource, + id: 'vm-platform-disks', + vmid: 103, + sources: ['proxmox', 'agent'], + proxmox: { disks: [disk('/platform')] }, + agent: { agentVersion: '6.4.5', stale: false, disks: [disk('/agent')] }, + }, + ] as any); + let dispose = () => {}; + let result: ReturnType | undefined; + createRoot((d) => { + dispose = d; + result = useWorkloads(() => true, { resourceSnapshot: snapshot }); + }); + try { + await flushAsync(); + const byId = (id: string) => + result!.workloads().find((workload) => workload.canonicalResourceId === id); + expect(byId('vm-agent-disks')).toMatchObject({ agentStale: true, disksFromAgent: true }); + expect(byId('vm-agent-disks')?.disks?.map((d) => d.mountpoint)).toEqual(['/agent']); + expect(byId('vm-platform-disks')?.agentStale).toBeUndefined(); + expect(byId('vm-platform-disks')?.disksFromAgent).toBeUndefined(); + expect(byId('vm-platform-disks')?.disks?.map((d) => d.mountpoint)).toEqual(['/platform']); + } finally { + dispose(); + } + }); + afterEach(() => { vi.useRealTimers(); vi.clearAllMocks(); diff --git a/frontend-modern/src/hooks/useWorkloads.ts b/frontend-modern/src/hooks/useWorkloads.ts index 35f238e8cb..f8d45872eb 100644 --- a/frontend-modern/src/hooks/useWorkloads.ts +++ b/frontend-modern/src/hooks/useWorkloads.ts @@ -146,6 +146,7 @@ type APIResource = { networkInterfaces?: APINetworkInterface[]; disks?: APIDiskInfo[]; raid?: HostRAIDArray[]; + stale?: boolean; }; docker?: { containerId?: string; @@ -592,6 +593,7 @@ const mapResourceToWorkload = (resource: APIResource): WorkloadGuest | null => { })(), disk: buildMetric(resource.metrics?.disk), disks: normalizeDiskArray(resource.proxmox?.disks ?? resource.agent?.disks), + disksFromAgent: (resource.proxmox?.disks == null && resource.agent?.disks != null) || undefined, // Both the API and owning platform snapshot pass through this mapper. // A retained numeric metric must not lose the provider's read deferral. diskStatusReason: resource.proxmox?.diskStatusReason, @@ -612,6 +614,9 @@ const mapResourceToWorkload = (resource: APIResource): WorkloadGuest | null => { agentId: resource.agent?.agentId, agentRaid: resource.agent?.raid, networkInterfaces: mapNetworkInterfaces(resource.agent?.networkInterfaces), + // A linked agent past its reporting lease keeps its last report on the + // resource, so its RAID, network and filesystem rows are not current. + agentStale: resource.agent?.stale === true || undefined, networkIn: finiteMetricNumber(resource.metrics?.netIn?.value) ?? 0, networkOut: finiteMetricNumber(resource.metrics?.netOut?.value) ?? 0, diskRead: finiteMetricNumber(resource.metrics?.diskRead?.value) ?? 0, diff --git a/frontend-modern/src/types/workloads.ts b/frontend-modern/src/types/workloads.ts index 411d3c3777..b4bb88c54e 100644 --- a/frontend-modern/src/types/workloads.ts +++ b/frontend-modern/src/types/workloads.ts @@ -60,6 +60,16 @@ export type WorkloadGuest = (VM | Container) & { agentId?: string; /** RAID arrays reported by the linked Pulse agent, not Proxmox virtual disks. */ agentRaid?: HostRAIDArray[]; + /** + * Set when the linked Pulse agent is past its reporting lease. Its RAID, + * network and filesystem readings are then its last report, not current. + */ + agentStale?: boolean; + /** + * Set when `disks` came from the linked Pulse agent because the platform + * reported no guest filesystems, so the rows follow that agent's freshness. + */ + disksFromAgent?: boolean; updateStatus?: DockerContainerUpdateStatus; // Server-evaluated capability refusals from the unified resource. The // update button reads this to disable itself with the refusal reason From 7a8376c055a4ccd8da954a39cac631f3b1066427 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 14:54:59 +0100 Subject: [PATCH 8/9] Keep platform metric alerts through Proxmox node cleanup CleanupAlertsForNodes runs on every Proxmox node poll and removes alerts whose node is not a current Proxmox node or PBS instance. Kubernetes, TrueNAS and vSphere metric alerts carry a cluster or platform host as their node and a display label such as "TrueNAS Disk" in metadata.resourceType, which the preserve list's platform words never matched. So in an estate with Proxmox and one of those platforms, every poll deleted these alerts silently while their reducer incident stayed firing, and the next evaluation re-created each one and handed it to the alert callback again as a new alert. Keep alerts that carry metadata.platformType, or whose label maps to a Kubernetes, TrueNAS or vSphere type. A guest alert on a removed Proxmox node is still removed. --- .../v6/internal/subsystems/alerts.md | 17 +++++ internal/alerts/active_cleanup.go | 10 +++ internal/alerts/alerts_test.go | 76 +++++++++++++++++++ 3 files changed, 103 insertions(+) diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index 2690c5f5ea..3509cfddf1 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -1842,6 +1842,23 @@ well as its alerts. `internal/alerts/host_unraid_lifecycle_test.go` pins the clear, the holds, the restarted count and the pending-run and count cleanup. +### Proxmox node cleanup keeps platform metric alerts + +`CleanupAlertsForNodes` runs on every Proxmox node poll (and every mock alert +tick) and removes alerts whose `Node` is not a current Proxmox node or PBS +instance. Kubernetes, TrueNAS and vSphere metric alerts carry a cluster or +platform host in `Node` and a display label such as `TrueNAS Disk` or +`Kubernetes Pod` in `metadata.resourceType`, which the preserve list's +platform words never matched. The cleanup used to delete them silently on +each poll while their reducer incident stayed firing, so the next evaluation +re-created each one and handed it to the alert callback again as a new +alert. `shouldPreserveAlertOutsideNodeCleanup` now keeps any alert carrying +`metadata.platformType`, or whose label maps through +`CanonicalAlertResourceType` to a Kubernetes, TrueNAS or vSphere type. +`TestCleanupAlertsForNodesKeepsPlatformMetricAlerts` in +`internal/alerts/alerts_test.go` pins both signals, and its +sibling pins that a guest alert on a removed Proxmox node is still removed. + ### Configured flapping thresholds remain reachable Every accepted positive `FlappingThreshold`, including values above ten, must diff --git a/internal/alerts/active_cleanup.go b/internal/alerts/active_cleanup.go index 4da24fa05d..2fdfcd34fc 100644 --- a/internal/alerts/active_cleanup.go +++ b/internal/alerts/active_cleanup.go @@ -4,6 +4,7 @@ import ( "strings" "time" + alertconfig "github.com/rcourtman/pulse-go-rewrite/internal/alerts/config" alertspecs "github.com/rcourtman/pulse-go-rewrite/internal/alerts/specs" "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" "github.com/rs/zerolog/log" @@ -264,6 +265,15 @@ func shouldPreserveAlertOutsideNodeCleanup(alertID string, alert *Alert) bool { if alert.CanonicalKind == string(alertspecs.AlertSpecKindProviderIncident) { return true } + // Kubernetes, TrueNAS and vSphere metric alerts carry a display label such + // as "TrueNAS Disk" or "Kubernetes Pod" in resourceType, which the platform + // words below never matched, and a cluster or host in Node rather than a + // Proxmox node. Every Proxmox poll used to delete them silently, and the + // next evaluation raised them again as new alerts. + if metadataStringValue(alert.Metadata, alertPlatformTypeKey) != "" || + isUnifiedModernPlatformAlertType(alertconfig.CanonicalAlertResourceType(alertMetadataResourceType(alert))) { + return true + } switch alertMetadataResourceType(alert) { case string(unifiedresources.ResourceTypeAgent), diff --git a/internal/alerts/alerts_test.go b/internal/alerts/alerts_test.go index f4c4463c5e..f0ad060e0f 100644 --- a/internal/alerts/alerts_test.go +++ b/internal/alerts/alerts_test.go @@ -12628,6 +12628,82 @@ func TestCleanupAlertsForNodes(t *testing.T) { }) } +// TestCleanupAlertsForNodesKeepsPlatformMetricAlerts pins that the Proxmox +// node cleanup, which runs on every Proxmox poll, leaves Kubernetes, TrueNAS +// and vSphere metric alerts alone. Their Node is a cluster or platform host, +// never a Proxmox node, and their resourceType is a display label. +func TestCleanupAlertsForNodesKeepsPlatformMetricAlerts(t *testing.T) { + m := newTestManager(t) + configureUnifiedEvalManager(t, m, unifiedEvalBaseConfig()) + + inputs := []*UnifiedResourceInput{ + {ID: "physical-disk:truenas-main/sda", Type: "truenas-disk", Name: "sda", Node: "truenas-main", Instance: "TrueNAS", Temperature: &UnifiedResourceMetric{Value: 80, Percent: 80}}, + {ID: "storage:truenas-main/pool:tank", Type: "truenas-pool", Name: "tank", Node: "truenas-main", Instance: "TrueNAS", Disk: &UnifiedResourceMetric{Percent: 95}}, + {ID: "k8s:prod:pod:api", Type: "pod", Name: "api", Node: "k8s-node-1", Instance: "prod", CPU: &UnifiedResourceMetric{Percent: 99}}, + {ID: "vmware:vc:vm:app-01", Type: "vmware-vm", Name: "app-01", Node: "esxi-01", Instance: "vc", CPU: &UnifiedResourceMetric{Percent: 99}}, + } + for _, input := range inputs { + m.CheckUnifiedResource(input) + } + platformAlerts := alertKeys(m) + if len(platformAlerts) != len(inputs) { + t.Fatalf("expected one alert per platform resource, got %v", platformAlerts) + } + + m.CleanupAlertsForNodes(map[string]bool{"pve1": true}) + + for _, alertID := range platformAlerts { + if _, exists := testLookupActiveAlert(t, m, alertID); !exists { + t.Fatalf("Proxmox node cleanup removed platform alert %q; remaining %v", alertID, alertKeys(m)) + } + } + + // The platform id alone keeps an alert whose label is not recognised. + m.mu.Lock() + labels := make(map[*Alert]interface{}, len(m.activeAlerts)) + for _, alert := range m.activeAlerts { + labels[alert] = alert.Metadata["resourceType"] + alert.Metadata["resourceType"] = "Unrecognised" + } + m.mu.Unlock() + m.CleanupAlertsForNodes(map[string]bool{"pve1": true}) + if got := alertKeys(m); len(got) != len(platformAlerts) { + t.Fatalf("platform alerts removed despite their platformType: %v", got) + } + m.mu.Lock() + for alert, label := range labels { + alert.Metadata["resourceType"] = label + } + m.mu.Unlock() + + // A legacy-shaped alert whose metadata predates platformType is still kept + // by its display label. + m.mu.Lock() + for _, alert := range m.activeAlerts { + delete(alert.Metadata, alertPlatformTypeKey) + } + m.mu.Unlock() + m.CleanupAlertsForNodes(map[string]bool{"pve1": true}) + if got := alertKeys(m); len(got) != len(platformAlerts) { + t.Fatalf("display-label platform alerts removed without platformType: %v", got) + } +} + +// The cleanup still removes a Proxmox guest alert whose node left the estate. +func TestCleanupAlertsForNodesStillRemovesMissingProxmoxNodeAlerts(t *testing.T) { + m := newTestManager(t) + configureUnifiedEvalManager(t, m, unifiedEvalBaseConfig()) + + m.CheckUnifiedResource(&UnifiedResourceInput{ID: "pve-gone:node-gone:101", Type: "vm", Name: "vm-101", Node: "node-gone", Instance: "pve-gone", CPU: &UnifiedResourceMetric{Percent: 99}}) + if len(alertKeys(m)) != 1 { + t.Fatalf("expected the guest CPU alert, got %v", alertKeys(m)) + } + m.CleanupAlertsForNodes(map[string]bool{"pve1": true}) + if got := alertKeys(m); len(got) != 0 { + t.Fatalf("guest alert on a removed node survived cleanup: %v", got) + } +} + func TestCheckZFSPoolHealth(t *testing.T) { // t.Parallel() poolStateAlertID := buildCanonicalStateID("local-zfs/zfs-pool:rpool", "local-zfs/zfs-pool:rpool-state") From 8676ec687ff902c8fa856363273494b7e535aecd Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 15:03:02 +0100 Subject: [PATCH 9/9] Read the shared wall clock on Alerts overview cards The overview hook kept its own minute signal and handed its value to each card as "now". A card that mounted between ticks measured from up to a minute earlier: an alert raised 70 seconds ago read "this minute", a reading already past the 10-minute stale cut-off led the card as the live one, a just-raised alert stayed out of Triggered (24h) until the next tick, and the stale flip and 24h drop landed up to a minute late. The card now reads useRelativeTimeNow for its started age and the live reading's stale cut-off, and the hook reads it for the 24h count, so they measure from the wall clock and move with the shared 30-second tick. The minute interval stays only for the delivery diagnoses refresh, a server read that should not double its rate. --- .../v6/internal/subsystems/alerts.md | 16 ++ .../subsystems/frontend-primitives.md | 6 + .../6324f927820d6ea5.json | 37 +++++ .../alerts/AlertOverviewAlertCard.tsx | 10 +- .../AlertOverviewAlertCard.clock.test.tsx | 152 ++++++++++++++++++ ...ertOverviewAlertCard.system-scope.test.tsx | 1 - .../__tests__/useAlertOverviewState.test.tsx | 17 ++ .../features/alerts/useAlertOverviewState.ts | 22 +-- 8 files changed, 245 insertions(+), 16 deletions(-) create mode 100644 frontend-modern/browser-verification/6324f927820d6ea5.json create mode 100644 frontend-modern/src/features/alerts/__tests__/AlertOverviewAlertCard.clock.test.tsx diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index 9d7946179e..6bf31e686c 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -2745,6 +2745,22 @@ derived alert read-model and Last 24 Hours stat refresh for shared acknowledgement owner instead of keeping its own alert mutation fork. Future overview action behavior should extend that shared acknowledgement hook instead of putting acknowledge mutations back into render shells. +Overview ages and bands read the shared clock. `AlertOverviewAlertCard.tsx` +passes its own `useRelativeTimeNow` reading as `now` to +`formatAlertOverviewStartedAgo` and `getMetricAlertPresentation`, and the Last +24 Hours count in `useAlertOverviewState.ts` reads the same clock, so all three +measure from the wall clock. The hook no longer exposes a minute `tick`: read +as "now", that tick measured a card mounted between ticks from up to a minute +earlier, so an alert raised 70 seconds ago read "this minute", an alert +raised since the last tick stayed out of the 24h count until the next one, a +reading already past the 10-minute stale cut-off led the card as live, and the +stale flip and 24h drop landed up to a minute late. A mounted card now turns stale, +and an alert leaves the 24h count, within one 30-second tick with no data +change. The delivery diagnoses refresh is a server read and keeps its own +minute interval rather than following the 30-second clock. +`AlertOverviewAlertCard.clock.test.tsx` pins the mid-tick mount, the stale flip +and the 24h drop under fake timers, and `useAlertOverviewState.test.tsx` pins +the diagnoses cadence. Render-heavy alert overview ownership now routes through `frontend-modern/src/features/alerts/AlertOverviewStatsCards.tsx`, `frontend-modern/src/features/alerts/AlertOverviewActiveAlertsSection.tsx`, diff --git a/docs/release-control/v6/internal/subsystems/frontend-primitives.md b/docs/release-control/v6/internal/subsystems/frontend-primitives.md index a5fa98dd52..f6598abcd8 100644 --- a/docs/release-control/v6/internal/subsystems/frontend-primitives.md +++ b/docs/release-control/v6/internal/subsystems/frontend-primitives.md @@ -7177,6 +7177,12 @@ shell stays in `frontend-modern/src/features/alerts/OverviewTab.tsx`, while alert stats, filtered ordering, and single/bulk acknowledge runtime behavior. Future overview control flow should extend that hook rather than restoring action timers or acknowledge mutations to the tab shell. +The overview follows the shared-clock rule for relative ages: the alert card +reads `useRelativeTimeNow` for its started age and the live reading's stale +cut-off, and the hook reads it for the Last 24 Hours count. A hook-local +minute signal used as "now" is a table-local interval under that rule; the +hook keeps a minute interval only to refresh delivery diagnoses, a server read +whose cadence is not tied to the age clock. Render-heavy overview ownership now lives in `frontend-modern/src/features/alerts/AlertOverviewStatsCards.tsx`, `frontend-modern/src/features/alerts/AlertOverviewActiveAlertsSection.tsx`, diff --git a/frontend-modern/browser-verification/6324f927820d6ea5.json b/frontend-modern/browser-verification/6324f927820d6ea5.json new file mode 100644 index 0000000000..2170db9557 --- /dev/null +++ b/frontend-modern/browser-verification/6324f927820d6ea5.json @@ -0,0 +1,37 @@ +{ + "version": 1, + "base_sha": "bab38a25b92684c978c4273200765152040624ed", + "verified_at": "2026-10-07T14:02:51Z", + "result": "passed", + "changed_paths": [ + "frontend-modern/src/features/alerts/AlertOverviewAlertCard.tsx", + "frontend-modern/src/features/alerts/useAlertOverviewState.ts" + ], + "content_sha256": { + "frontend-modern/src/features/alerts/AlertOverviewAlertCard.tsx": "bb98293f7a7b7b857b40ec71a38e1e32163aa937405278dd07af2a8c9dac1118", + "frontend-modern/src/features/alerts/useAlertOverviewState.ts": "3e2c91ba930a1d3faf3965a5fd68f5bb44e27465a88cbb06c23aabe500d25668" + }, + "routes": [ + "/alerts/overview" + ], + "viewports": [ + { + "width": 1440, + "height": 900 + }, + { + "width": 390, + "height": 844 + } + ], + "states": [ + "Private mock stack (backend :7697, Vite :5217 serving this tree on base bab38a25b9) with Playwright page.clock installed before load and an init script recording when each setInterval starts, so the shared 30s clock and the overview hook's minute interval have known phases", + "Three producer-shaped copies of a live disk threshold alert (metricStatus breaching) sent in one websocket frame 15s after a shared tick and 45s after the hook's minute, then /ws data frames held (data-free pong per step) and /api reads stalled, so only the clock moves", + "Late-mounted card (raised 70s ago, reading 10m05s old) reads '1 min. ago' and 'Last reading: Disk 92%, 10 mins ago' at mount; held card (reading goes stale 5s after the hook's next minute) is live at mount and 1s before the cut-off, and reads 'Last reading: ..., 10 mins ago' 31s after it; Triggered (24h) drops 85 -> 84 31s after the third copy turns 24h old; no page errors; same at 1440 and 390", + "Same-session control with both runtime files swapped to main under the running Vite: the late card read 'this minute' and led with the old reading as live, the held card was still live 31s after the cut-off, and the 24h count rose 84 -> 85 because the just-raised copy read as future; fix restored and diff compared byte-for-byte", + "tsc clean; src/features/alerts, src/components/Alerts and src/utils suites pass except the Mac-only agentInstallCommand lifecycle tests" + ], + "interactions": [ + "Loaded /alerts/overview, injected the alert copies between ticks, held the stream and stepped the page clock past the stale cut-off and the 24h boundary, at both viewports, before and after the fix" + ] +} diff --git a/frontend-modern/src/features/alerts/AlertOverviewAlertCard.tsx b/frontend-modern/src/features/alerts/AlertOverviewAlertCard.tsx index 870d095c84..cd8487f0c7 100644 --- a/frontend-modern/src/features/alerts/AlertOverviewAlertCard.tsx +++ b/frontend-modern/src/features/alerts/AlertOverviewAlertCard.tsx @@ -36,6 +36,7 @@ import { AlertSnoozeAction } from './AlertSnoozeAction'; import { isAlertSnoozed } from './useAlertSnoozeState'; import { isPulseSystemAlert } from '@/utils/alertScope'; import { formatTemperature } from '@/utils/temperature'; +import { useRelativeTimeNow } from '@/utils/relativeTimeClock'; import { normalizeSourcePlatformQueryValue, resolvePlatformTypeFromSources, @@ -110,6 +111,9 @@ const platformPagePath = (platform: string | undefined): string | undefined => { }; export function AlertOverviewAlertCard(props: AlertOverviewAlertCardProps) { + // The started age and the live reading's stale cut-off and ages measure + // from the wall clock, so a card that mounts between ticks is current. + const now = useRelativeTimeNow(); const alertKey = () => getCanonicalAlertId(props.alert); const hasResource = () => !isPulseSystemAlert(props.alert) && Boolean(props.alert.resourceId?.trim()); @@ -138,9 +142,7 @@ export function AlertOverviewAlertCard(props: AlertOverviewAlertCardProps) { // A threshold alert can stay open below its trigger, so the card leads with // the reading Pulse is evaluating now; the message keeps the last breach. const metricPresentation = () => - isPulseSystemAlert(props.alert) - ? null - : getMetricAlertPresentation(props.alert, props.state.tick()); + isPulseSystemAlert(props.alert) ? null : getMetricAlertPresentation(props.alert, now()); const alertLevels = (): { alert: string; clear?: string } | null => { const presentation = metricPresentation(); if (presentation) { @@ -318,7 +320,7 @@ export function AlertOverviewAlertCard(props: AlertOverviewAlertCardProps) { new Date(props.alert.startTime).toLocaleString(), )} > - {formatAlertOverviewStartedAgo(props.alert.startTime, props.state.tick())} + {formatAlertOverviewStartedAgo(props.alert.startTime, now())}
({ + useLocation: () => ({ hash: '', pathname: '/alerts', search: '', query: {} }), + A: (props: Record) => props.children, +})); + +const getDeliveryDiagnoses = vi.fn<() => Promise>(); + +vi.mock('@/api/alerts', () => ({ + AlertsAPI: { + get getDeliveryDiagnoses() { + return getDeliveryDiagnoses; + }, + }, +})); + +vi.mock('@/stores/notifications', () => ({ + notificationStore: { success: vi.fn(), error: vi.fn() }, +})); + +vi.mock('@/utils/logger', () => ({ + logger: { error: vi.fn() }, +})); + +vi.mock('@/components/Alerts/InvestigateAlertButton', () => ({ + InvestigateAlertButton: () => null, +})); + +import { OverviewTab } from '../OverviewTab'; + +const OPENED_AT = Date.parse('2026-10-07T12:00:00Z'); +// Between the shared clock's first and second ticks, and before the overview's +// own minute. +const BETWEEN_TICKS_MS = RELATIVE_TIME_TICK_MS + 15_000; + +function cpuAlert(id: string, startTime: number, observedAt: number): Alert { + return { + id, + resourceId: `vm-${id}`, + resourceName: `VM ${id}`, + type: 'cpu', + level: 'warning', + message: `CPU on VM ${id} at 92%`, + value: 92, + threshold: 80, + startTime: new Date(startTime).toISOString(), + lastSeen: new Date(observedAt).toISOString(), + acknowledged: false, + node: 'node1', + metricStatus: { + phase: 'breaching', + value: 91, + unit: '%', + observedAt: new Date(observedAt).toISOString(), + trigger: 80, + recovery: 75, + }, + } as Alert; +} + +function renderOverview() { + const [activeAlerts, setActiveAlerts] = createSignal>({}); + render(() => ( + false} + dismissQuickTip={vi.fn()} + showAcknowledged={() => true} + setShowAcknowledged={vi.fn()} + alertsDisabled={() => false} + /> + )); + return setActiveAlerts; +} + +const triggered24h = () => + screen + .getByText('Triggered (24h)') + .closest('[data-alert-overview-stat]') + ?.querySelector('[data-testid="alert-overview-stat-value"]')?.textContent; + +describe('Alert overview card clock', () => { + beforeEach(() => { + setActiveLocale(DEFAULT_LOCALE); + getDeliveryDiagnoses.mockReset(); + getDeliveryDiagnoses.mockResolvedValue([]); + vi.useFakeTimers(); + vi.setSystemTime(OPENED_AT); + }); + + afterEach(() => { + cleanup(); + vi.useRealTimers(); + }); + + it('measures a card that mounts between ticks from the wall clock', () => { + const setActiveAlerts = renderOverview(); + vi.advanceTimersByTime(BETWEEN_TICKS_MS); + + // Raised 70 seconds ago; last evaluated just past the stale cut-off. Read + // from the time of the last tick, the card would call it "this minute" + // and lead with the old reading as the live one. + const now = Date.now(); + setActiveAlerts({ + late: cpuAlert('late', now - 70_000, now - METRIC_ALERT_STATUS_STALE_MS - 5_000), + }); + + expect(screen.getByText('1 min. ago')).toBeInTheDocument(); + expect(screen.getByText('Last reading: CPU 91%, 10 mins ago')).toBeInTheDocument(); + }); + + it('turns stale at the next shared tick after the cut-off with no data change', () => { + const setActiveAlerts = renderOverview(); + vi.advanceTimersByTime(BETWEEN_TICKS_MS); + + // The reading goes stale 5 seconds after the shared clock's second tick. + const staleAt = OPENED_AT + 2 * RELATIVE_TIME_TICK_MS + 5_000; + const now = Date.now(); + setActiveAlerts({ held: cpuAlert('held', now, staleAt - METRIC_ALERT_STATUS_STALE_MS) }); + expect(screen.getByText('CPU 91%, above the 80% alert level')).toBeInTheDocument(); + + vi.advanceTimersByTime(OPENED_AT + 2 * RELATIVE_TIME_TICK_MS - Date.now()); + expect(screen.getByText('CPU 91%, above the 80% alert level')).toBeInTheDocument(); + + // The next tick lands within one shared tick of the cut-off, not on the + // overview's next minute. + vi.advanceTimersByTime(RELATIVE_TIME_TICK_MS); + expect(screen.queryByText('CPU 91%, above the 80% alert level')).toBeNull(); + expect(screen.getByText('Last reading: CPU 91%, 10 mins ago')).toBeInTheDocument(); + }); + + it('drops an alert from the 24h count at the next shared tick after it turns 24h old', () => { + const setActiveAlerts = renderOverview(); + vi.advanceTimersByTime(BETWEEN_TICKS_MS); + + const leavesWindowAt = OPENED_AT + 2 * RELATIVE_TIME_TICK_MS + 5_000; + setActiveAlerts({ old: cpuAlert('old', leavesWindowAt - 86_400_000, Date.now()) }); + expect(triggered24h()).toBe('1'); + + vi.advanceTimersByTime(OPENED_AT + 3 * RELATIVE_TIME_TICK_MS - Date.now()); + expect(triggered24h()).toBe('0'); + }); +}); diff --git a/frontend-modern/src/features/alerts/__tests__/AlertOverviewAlertCard.system-scope.test.tsx b/frontend-modern/src/features/alerts/__tests__/AlertOverviewAlertCard.system-scope.test.tsx index a3dddd8e8f..aae6143db3 100644 --- a/frontend-modern/src/features/alerts/__tests__/AlertOverviewAlertCard.system-scope.test.tsx +++ b/frontend-modern/src/features/alerts/__tests__/AlertOverviewAlertCard.system-scope.test.tsx @@ -26,7 +26,6 @@ import { AlertOverviewAlertCard } from '../AlertOverviewAlertCard'; const acknowledge = vi.fn(); const toggleTimeline = vi.fn(); const state = { - tick: () => Date.now(), processingAlerts: () => new Set(), snoozeProcessingAlerts: () => new Set(), deliveryDiagnoses: () => ({}), diff --git a/frontend-modern/src/features/alerts/__tests__/useAlertOverviewState.test.tsx b/frontend-modern/src/features/alerts/__tests__/useAlertOverviewState.test.tsx index 80e0cd2034..6c854746b4 100644 --- a/frontend-modern/src/features/alerts/__tests__/useAlertOverviewState.test.tsx +++ b/frontend-modern/src/features/alerts/__tests__/useAlertOverviewState.test.tsx @@ -6,6 +6,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { AlertsAPI } from '@/api/alerts'; import { notificationStore } from '@/stores/notifications'; import type { Alert, AlertDeliveryDiagnosis } from '@/types/api'; +import { RELATIVE_TIME_TICK_MS } from '@/utils/relativeTimeClock'; import { useAlertOverviewState } from '../useAlertOverviewState'; @@ -118,6 +119,22 @@ describe('useAlertOverviewState', () => { expect(result.deliveryDiagnoses()).toEqual({ a1: retained }); }); + it('refreshes diagnoses once a minute, not on the 30-second age clock', async () => { + renderHook(() => + useAlertOverviewState({ + activeAlerts: () => ({ a1: makeAlert('a1', new Date().toISOString()) }), + overrides: () => [], + showAcknowledged: () => true, + updateAlert: vi.fn(), + }), + ); + expect(AlertsAPI.getDeliveryDiagnoses).toHaveBeenCalledOnce(); + await vi.advanceTimersByTimeAsync(RELATIVE_TIME_TICK_MS); + expect(AlertsAPI.getDeliveryDiagnoses).toHaveBeenCalledOnce(); + await vi.advanceTimersByTimeAsync(60_000 - RELATIVE_TIME_TICK_MS); + expect(AlertsAPI.getDeliveryDiagnoses).toHaveBeenCalledTimes(2); + }); + it('ignores pending responses and stops periodic reads after disposal', async () => { let finish!: (value: AlertDeliveryDiagnosis[]) => void; vi.mocked(AlertsAPI.getDeliveryDiagnoses).mockReturnValueOnce( diff --git a/frontend-modern/src/features/alerts/useAlertOverviewState.ts b/frontend-modern/src/features/alerts/useAlertOverviewState.ts index 583bb9163b..0424aba4da 100644 --- a/frontend-modern/src/features/alerts/useAlertOverviewState.ts +++ b/frontend-modern/src/features/alerts/useAlertOverviewState.ts @@ -3,6 +3,7 @@ import type { Accessor } from 'solid-js'; import { AlertsAPI } from '@/api/alerts'; import type { Alert, AlertDeliveryDiagnosis } from '@/types/api'; +import { useRelativeTimeNow } from '@/utils/relativeTimeClock'; import type { Override } from './types'; import { useAlertAcknowledgementState } from './useAlertAcknowledgementState'; import { useAlertSnoozeState } from './useAlertSnoozeState'; @@ -34,8 +35,7 @@ export interface UseAlertOverviewStateProps { } export function useAlertOverviewState(props: UseAlertOverviewStateProps) { - const [tick, setTick] = createSignal(Date.now()); - const tickInterval = setInterval(() => setTick(Date.now()), 60_000); + const now = useRelativeTimeNow(); const activeAlerts = createMemo(() => Object.values(props.activeAlerts())); const { unacknowledgedAlerts, @@ -56,10 +56,6 @@ export function useAlertOverviewState(props: UseAlertOverviewStateProps) { handleUnsnooze, } = useAlertSnoozeState({ alerts: activeAlerts, updateAlert: props.updateAlert }); - onCleanup(() => { - clearInterval(tickInterval); - }); - // Delivery diagnoses answer "did/will this alert notify?" per card. One // bulk request covers every active alert; a failed refresh keeps the last // snapshot and the card simply shows no delivery line for unknown alerts. @@ -98,19 +94,24 @@ export function useAlertOverviewState(props: UseAlertOverviewStateProps) { .join('\n'), ); createEffect(() => { - // Refresh when the active alert set changes and on the shared minute - // tick, so time-based holds (cooldown, quiet hours) stay current. + // Refresh when the active alert set changes. activeAlertIdsKey(); - tick(); void refreshDeliveryDiagnoses(); }); + // And once a minute, so time-based holds (cooldown, quiet hours) stay + // current. This is a server read, so it keeps its own minute rather than + // following the 30-second clock the ages read. + const diagnosisInterval = setInterval(() => void refreshDeliveryDiagnoses(), 60_000); + onCleanup(() => { + clearInterval(diagnosisInterval); + }); const alertStats = createMemo(() => { const alerts = effectiveAlerts(); const recent = alerts.filter((alert) => { const ts = new Date(alert.startTime).getTime(); if (Number.isNaN(ts)) return true; - const age = tick() - ts; + const age = now() - ts; return age >= 0 && age < 86_400_000; }); const unacknowledged = alerts.filter((alert) => !alert.acknowledged); @@ -169,7 +170,6 @@ export function useAlertOverviewState(props: UseAlertOverviewStateProps) { }); return { - tick, alertStats, filteredAlerts, groupedAlerts,