From 278c403d9674cb2eb2066707637e88770636658b Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Tue, 6 Oct 2026 16:46:32 +0100 Subject: [PATCH 01/12] Keep watchdog, delivery log and Patrol ages moving The shared relative-time clock reached the platform tables, but several long-lived Alerts and Patrol panels still computed ages once at render and froze while the panel stayed open over data that did not change. The external watchdog panel read its status once at mount, so Last success and Monitor progress kept their read-time age through a real stall, which is the case the panel exists to show. It now re-reads the status every 30 seconds in the background, leaving the Refresh control alone, and both ages read the clock. The delivery log's hover age reads the clock too: a logged attempt's time never changes. Patrol's paused banner kept "Blocked 2m ago" because each status re-read returns the same blocked time, and the selected-run banner kept its age because the selected run is held as picked. The attention detail is read once per selection, so its acknowledgement, transition and evidence times now read the clock. Its Last seen and flapping window describe a record that may have moved since, so they stay read-time. Queue rows, summary, run history and the digest already re-derive on their own 30- or 60-second re-reads and are unchanged. --- .../v6/internal/subsystems/alerts.md | 11 +++ .../v6/internal/subsystems/api-contracts.md | 6 ++ .../subsystems/frontend-primitives.md | 9 ++ .../subsystems/patrol-intelligence.md | 11 +++ .../530561c68f6c71e9.json | 51 ++++++++++ .../alerts/AlertDeadManDestinationSection.tsx | 39 ++++++-- .../alerts/AlertDeliveryLogCard.test.tsx | 33 +++++++ .../features/alerts/AlertDeliveryLogCard.tsx | 8 +- .../AlertDeadManDestinationSection.test.tsx | 43 ++++++++- .../patrol/PatrolAttentionWorkbench.tsx | 16 +++- .../patrol/PatrolIntelligenceBanners.tsx | 6 +- .../patrol/PatrolIntelligenceWorkspace.tsx | 6 +- .../PatrolAttentionWorkbench.test.tsx | 40 ++++++++ .../PatrolRunBanners.relativeAge.test.tsx | 96 +++++++++++++++++++ 14 files changed, 359 insertions(+), 16 deletions(-) create mode 100644 frontend-modern/browser-verification/530561c68f6c71e9.json create mode 100644 frontend-modern/src/features/patrol/__tests__/PatrolRunBanners.relativeAge.test.tsx diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index 199b9bdc55..2cf5789a5a 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -3041,6 +3041,17 @@ delivery pause because those policies must not disable observation of Pulse. Its Refresh and Show/Hide actions are the shared outline `Button` (xs and `settingsAction` sizes), not a copied border shell, so they match the other destination controls and keep a 44px touch target on phones. +Last success and Monitor progress are ages, so they read the frontend-primitives +shared relative-time clock and keep moving while the panel stays open, and the +panel re-reads `GET /api/alerts/deadman/status` in the background every 30 +seconds. A moving age over a status read once at mount would claim the +heartbeat stopped while Pulse kept pinging every minute, and a status that is +never re-read kept its read-time age through a real stall, which is the one +case this panel exists to show. A background re-read leaves the Refresh control +alone, and a failed one keeps the last status and marks it unavailable. +The delivery log's relative hover time on each row reads the same clock: a +logged attempt's time never changes, so its age is correct over a log that is +only re-read on demand. Watchdog transport and monitoring progress remain notifications- and monitoring-owned respectively. Alerts owns the system-alert projection: diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index e410966406..52de395abb 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -4835,6 +4835,12 @@ clients must not treat their absence as evidence of stability. `frontend-modern/src/api/patrolAttention.ts` (`AttentionFlapping`) and `frontend-modern/src/api/patrol.ts` (`FindingFlapping`, `mirrors_alert_*`) are the typed client boundary. +The Patrol page's paused banner, selected-run banner and attention detail +measure their ages (`blocked_at`, a run's `started_at`, acknowledgement, +transition and evidence times) from the frontend-primitives shared +relative-time clock. That is presentation over the same `GET +/api/ai/patrol/status`, run history and `GET /api/ai/patrol/attention/{id}` +payloads: no field, request or polling cadence changes. ### Resource-list facets preserve scoped navigation evidence diff --git a/docs/release-control/v6/internal/subsystems/frontend-primitives.md b/docs/release-control/v6/internal/subsystems/frontend-primitives.md index deb19c788a..efe7ea8740 100644 --- a/docs/release-control/v6/internal/subsystems/frontend-primitives.md +++ b/docs/release-control/v6/internal/subsystems/frontend-primitives.md @@ -5212,6 +5212,15 @@ derived from the same time (replication Next sync, backup age bands) reads it too. The clock can trail the wall clock by up to one tick, so a check that treats a future time as invalid (the backup age band) measures from the later of the two. +The rule has one deliberate exception. The age of a latest reading (last used, +last seen, last success, last checked) on data the surface reads once and does +not re-read stays the age at read time, because a moving age over a snapshot +that never refreshes claims the reading stopped when it may not have. Such a +surface either re-reads the snapshot in the background, as Proxmox replication +and the external watchdog panel do, or keeps the read-time age, as the Patrol +attention detail does for Last seen. An immutable event time (when a delivery +was attempted, a transition happened, a policy was set) ages correctly over any +snapshot and reads the clock. Read-only metadata badges follow the same primitive-owned shell rule. `frontend-modern/src/components/shared/MetadataBadge.tsx` owns filled and outlined appearances, compact sizing, shape, typed tone vocabulary, fit diff --git a/docs/release-control/v6/internal/subsystems/patrol-intelligence.md b/docs/release-control/v6/internal/subsystems/patrol-intelligence.md index 383727f107..a89b6a45e6 100644 --- a/docs/release-control/v6/internal/subsystems/patrol-intelligence.md +++ b/docs/release-control/v6/internal/subsystems/patrol-intelligence.md @@ -2647,6 +2647,17 @@ finding and carried onto the unified finding as `failureCause`, and a provider preflight success no longer clears a budget-exhausted runtime finding, so the setup card still routes to the budget after a restart clears the in-memory block state. +The paused banner's `Blocked ...` age reads the frontend-primitives shared +relative-time clock: each status re-read returns the same `blocked_at` while +Patrol stays paused, so the banner kept the age it had when Patrol first +paused. The selected run's `Patrol run ...` banner reads the clock for the same +reason, since the selected run stays as picked when run history is re-read. The +attention detail is read once per selection, so its fixed event times (the +acknowledgement, each timeline transition, each evidence observation) read the +clock while it stays open. Its `Last seen` and flapping window are latest +readings of a record that may have moved since, so they stay the ages at +read time, and the queue rows, summary, recent work, run history and digest +re-derive on their own 30- or 60-second re-reads. ### This week card answers what Patrol did for the customer diff --git a/frontend-modern/browser-verification/530561c68f6c71e9.json b/frontend-modern/browser-verification/530561c68f6c71e9.json new file mode 100644 index 0000000000..0a97bb0a6a --- /dev/null +++ b/frontend-modern/browser-verification/530561c68f6c71e9.json @@ -0,0 +1,51 @@ +{ + "version": 1, + "base_sha": "bffd21f47026ca2408ee1d0b5bb071c02e60c623", + "verified_at": "2026-10-06T15:46:26Z", + "result": "passed", + "changed_paths": [ + "frontend-modern/src/features/alerts/AlertDeadManDestinationSection.tsx", + "frontend-modern/src/features/alerts/AlertDeliveryLogCard.tsx", + "frontend-modern/src/features/patrol/PatrolAttentionWorkbench.tsx", + "frontend-modern/src/features/patrol/PatrolIntelligenceBanners.tsx", + "frontend-modern/src/features/patrol/PatrolIntelligenceWorkspace.tsx" + ], + "content_sha256": { + "frontend-modern/src/features/alerts/AlertDeadManDestinationSection.tsx": "df5d337cbac9e091aa0e9dfaa2623a7169eedc7609829a76338f0e4208685e0b", + "frontend-modern/src/features/alerts/AlertDeliveryLogCard.tsx": "90ab80e79b7516c5693768657739ab1b6426615312e0dc03ff9c8636f39f35af", + "frontend-modern/src/features/patrol/PatrolAttentionWorkbench.tsx": "7cbb73c2b4e903ba9bb4c783fada2493189f325f312c23a13f600541520e8a79", + "frontend-modern/src/features/patrol/PatrolIntelligenceBanners.tsx": "2c5818b026c1976786bf858fddcbbce552217eed4410302db573cbebef3ac944", + "frontend-modern/src/features/patrol/PatrolIntelligenceWorkspace.tsx": "5d68a4b1e2904bdfb76245bfda24f77bd330424f830952b4d134b1931fba0518" + }, + "routes": [ + "/alerts/destinations", + "/patrol", + "/patrol/activity" + ], + "viewports": [ + { + "width": 1440, + "height": 900 + }, + { + "width": 390, + "height": 844 + } + ], + "states": [ + "Private mock stack (backend :7731 built from this clone, Vite :5251 serving this branch, base bffd21f470, after #2578 landed) with Playwright page.clock installed before load; data held by dropping /ws data frames and replaying cached API GETs while a data-free keep-alive frame per fake minute keeps one live socket (no heartbeat reconnect)", + "Alerts > Destinations external watchdog with an injected healthy status (lastSuccessAt 30s, lastMonitoringProgress 20s before load; the mock has no watchdog): Last success 33s ago and Monitor progress 23s ago read 10 mins ago after 10 held minutes at 1440 and 390; 20 background re-reads of /api/alerts/deadman/status in those 10 minutes, Refresh button untouched", + "Same page, delivery log with one injected sent email attempt 5 minutes old: the row time tooltip moved 5 mins ago to 15 mins ago", + "Patrol activity with an injected blocked status (blocked_at 2 minutes before load): 'Blocked 2m ago' read 'Blocked 12m ago' after 10 held minutes", + "Patrol activity > Finding options and history > History, newest demo run start rewritten to 3 minutes before load: row read 3m ago then 13m ago; selecting it showed 'Patrol run 3m ago' and then 'Patrol run 13m ago' while held", + "Patrol inbox attention detail with injected acknowledgement, timeline and evidence times (3, 5 and 7 minutes before load): read 13m, 15m and 17m ago after 10 held minutes; 'Last seen' and the flapping window stay read-time by design", + "Same-session control: with these five runtime files swapped back to origin/main under the running Vite, the watchdog ages, delivery tooltip, Blocked banner, selected-run banner and attention detail ages stayed frozen (one status read, no background re-reads); only the run history row moved, because history re-reads on Patrol's 60-second refresh, so that row is not changed here", + "No page errors at either viewport" + ], + "interactions": [ + "Loaded /alerts/destinations at 1440x900 and 390x844, held the data stream and advanced the page clock ten one-minute steps", + "Loaded /patrol/activity, opened Finding options and history, opened History, selected the newest run, held and advanced ten minutes", + "Loaded /patrol, opened the first attention item's detail, held and advanced ten minutes", + "Repeated every scenario with the pre-change files as the control" + ] +} diff --git a/frontend-modern/src/features/alerts/AlertDeadManDestinationSection.tsx b/frontend-modern/src/features/alerts/AlertDeadManDestinationSection.tsx index 83174ca881..b6e5f53ee1 100644 --- a/frontend-modern/src/features/alerts/AlertDeadManDestinationSection.tsx +++ b/frontend-modern/src/features/alerts/AlertDeadManDestinationSection.tsx @@ -1,4 +1,4 @@ -import { createSignal, createUniqueId, onMount, Show, type Accessor } from 'solid-js'; +import { createSignal, createUniqueId, onCleanup, onMount, Show, type Accessor } from 'solid-js'; import { AlertsAPI } from '@/api/alerts'; import { Button } from '@/components/shared/Button'; @@ -6,6 +6,7 @@ import { SettingsPanel } from '@/components/shared/SettingsPanel'; import type { DeadManStatus } from '@/types/alerts'; import { formatRelativeTime } from '@/utils/format'; import { logger } from '@/utils/logger'; +import { useRelativeTimeNow } from '@/utils/relativeTimeClock'; interface AlertDeadManDestinationSectionProps { pingUrl: Accessor; @@ -14,6 +15,9 @@ interface AlertDeadManDestinationSectionProps { } const REDACTED_PING_URL = '***REDACTED***'; +// Pulse pings the watchdog every minute, so a status read once at mount goes +// stale while the panel stays open. +export const DEAD_MAN_STATUS_POLL_MS = 30_000; const statusPresentation: Record = { disabled: { @@ -54,9 +58,13 @@ export function AlertDeadManDestinationSection(props: AlertDeadManDestinationSec const [unavailable, setUnavailable] = createSignal(false); const [showUrl, setShowUrl] = createSignal(false); - const loadStatus = async () => { - if (loading()) return; - setLoading(true); + let backgroundLoad = false; + const loadStatus = async (options: { background?: boolean } = {}) => { + if (loading() || backgroundLoad) return; + // A background re-read leaves the Refresh button alone; a failed one keeps + // the last status and marks it unavailable. + if (options.background) backgroundLoad = true; + else setLoading(true); try { setStatus(await AlertsAPI.getDeadManStatus()); setUnavailable(false); @@ -64,11 +72,20 @@ export function AlertDeadManDestinationSection(props: AlertDeadManDestinationSec logger.error('Failed to load external watchdog status', error); setUnavailable(true); } finally { - setLoading(false); + if (options.background) backgroundLoad = false; + else setLoading(false); } }; - onMount(() => void loadStatus()); + // Last success and Monitor progress keep aging on the shared clock, and the + // status is re-read in the background so a moving age never describes a + // heartbeat that has since been sent. + const now = useRelativeTimeNow(); + onMount(() => { + void loadStatus(); + const timer = setInterval(() => void loadStatus({ background: true }), DEAD_MAN_STATUS_POLL_MS); + onCleanup(() => clearInterval(timer)); + }); const presentation = () => { const current = status(); @@ -162,7 +179,10 @@ export function AlertDeadManDestinationSection(props: AlertDeadManDestinationSec
Last success
{current().lastSuccessAt - ? formatRelativeTime(current().lastSuccessAt, { emptyText: 'Never' }) + ? formatRelativeTime(current().lastSuccessAt, { + emptyText: 'Never', + now: now(), + }) : 'Never'}
@@ -170,7 +190,10 @@ export function AlertDeadManDestinationSection(props: AlertDeadManDestinationSec
Monitor progress
{current().lastMonitoringProgress - ? formatRelativeTime(current().lastMonitoringProgress, { emptyText: 'Waiting' }) + ? formatRelativeTime(current().lastMonitoringProgress, { + emptyText: 'Waiting', + now: now(), + }) : 'Waiting'}
diff --git a/frontend-modern/src/features/alerts/AlertDeliveryLogCard.test.tsx b/frontend-modern/src/features/alerts/AlertDeliveryLogCard.test.tsx index 4fb6b60f9a..47d20243a8 100644 --- a/frontend-modern/src/features/alerts/AlertDeliveryLogCard.test.tsx +++ b/frontend-modern/src/features/alerts/AlertDeliveryLogCard.test.tsx @@ -4,6 +4,7 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; import type { NotificationDeliveryLog, Webhook } from '@/api/notifications'; import type { AlertEvent } from '@/types/api'; +import { RELATIVE_TIME_TICK_MS } from '@/utils/relativeTimeClock'; import { AlertDeliveryLogCard, mergeDeliveryLogRows } from './AlertDeliveryLogCard'; @@ -437,4 +438,36 @@ describe('AlertDeliveryLogCard', () => { expect(screen.getByRole('status')).toHaveTextContent('Loading delivery attempts...'); expect(screen.queryByText(/No alert deliveries were attempted/)).not.toBeInTheDocument(); }); + + it("keeps each row's age tooltip moving while the log is not refreshed", () => { + vi.useFakeTimers({ + toFake: ['Date', 'setInterval', 'clearInterval'], + now: Date.parse('2026-08-27T12:05:00Z'), + }); + + try { + render(() => ( + + )); + const time = () => document.querySelector('time[datetime="2026-08-27T12:00:00Z"]'); + + expect(time()).toHaveAttribute('title', '5 mins ago'); + + // The log is not re-read: the entry never changes and only the clock moves. + vi.advanceTimersByTime(2 * 60 * 2 * RELATIVE_TIME_TICK_MS); + + expect(time()).toHaveAttribute('title', '2 hours ago'); + } finally { + vi.useRealTimers(); + } + }); }); diff --git a/frontend-modern/src/features/alerts/AlertDeliveryLogCard.tsx b/frontend-modern/src/features/alerts/AlertDeliveryLogCard.tsx index cb1be99e27..74953bfc06 100644 --- a/frontend-modern/src/features/alerts/AlertDeliveryLogCard.tsx +++ b/frontend-modern/src/features/alerts/AlertDeliveryLogCard.tsx @@ -21,6 +21,7 @@ import { getAlertDestinationsDeliveryRefreshLabel, } from '@/utils/alertDestinationsPresentation'; import { formatRelativeTime } from '@/utils/format'; +import { useRelativeTimeNow } from '@/utils/relativeTimeClock'; import { describeAlertEventReason } from './deliveryDiagnosisPresentation'; @@ -76,6 +77,9 @@ const outcomeBadgeClasses: Record { @@ -215,7 +219,7 @@ export function AlertDeliveryLogCard(props: AlertDeliveryLogCardProps) { @@ -259,7 +263,7 @@ export function AlertDeliveryLogCard(props: AlertDeliveryLogCardProps) { diff --git a/frontend-modern/src/features/alerts/__tests__/AlertDeadManDestinationSection.test.tsx b/frontend-modern/src/features/alerts/__tests__/AlertDeadManDestinationSection.test.tsx index 7b06fb4601..17ba5f8165 100644 --- a/frontend-modern/src/features/alerts/__tests__/AlertDeadManDestinationSection.test.tsx +++ b/frontend-modern/src/features/alerts/__tests__/AlertDeadManDestinationSection.test.tsx @@ -4,7 +4,10 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { AlertsAPI } from '@/api/alerts'; -import { AlertDeadManDestinationSection } from '../AlertDeadManDestinationSection'; +import { + AlertDeadManDestinationSection, + DEAD_MAN_STATUS_POLL_MS, +} from '../AlertDeadManDestinationSection'; vi.mock('@/api/alerts', () => ({ AlertsAPI: { @@ -113,4 +116,42 @@ describe('AlertDeadManDestinationSection', () => { screen.getByText('Saved external watchdog configuration could not be read'), ).toBeInTheDocument(); }); + + it('keeps Last success aging and re-reads the status while the panel stays open', async () => { + const start = Date.parse('2026-08-27T12:00:30Z'); + vi.useFakeTimers({ toFake: ['Date', 'setInterval', 'clearInterval'], now: start }); + const [pingUrl, setPingUrl] = createSignal('***REDACTED***'); + + try { + render(() => ( + + )); + const lastSuccess = () => screen.getByText('Last success').nextElementSibling; + await waitFor(() => expect(lastSuccess()).toHaveTextContent('30s ago')); + expect(AlertsAPI.getDeadManStatus).toHaveBeenCalledTimes(1); + + // The watchdog stops being reached: every background re-read returns the + // same last success, and its age keeps moving on the clock. + for (let poll = 2; poll <= 5; poll += 1) { + vi.advanceTimersByTime(DEAD_MAN_STATUS_POLL_MS); + await waitFor(() => expect(AlertsAPI.getDeadManStatus).toHaveBeenCalledTimes(poll)); + } + expect(lastSuccess()).toHaveTextContent('2 mins ago'); + expect(screen.getByRole('button', { name: 'Refresh' })).not.toBeDisabled(); + + // A later heartbeat lands: the next background re-read picks it up. + vi.mocked(AlertsAPI.getDeadManStatus).mockResolvedValue({ + ...(await AlertsAPI.getDeadManStatus()), + lastSuccessAt: '2026-08-27T12:02:30Z', + }); + vi.advanceTimersByTime(DEAD_MAN_STATUS_POLL_MS); + await waitFor(() => expect(lastSuccess()).toHaveTextContent('30s ago')); + } finally { + vi.useRealTimers(); + } + }); }); diff --git a/frontend-modern/src/features/patrol/PatrolAttentionWorkbench.tsx b/frontend-modern/src/features/patrol/PatrolAttentionWorkbench.tsx index 76bbcbe03d..15fb0c12c1 100644 --- a/frontend-modern/src/features/patrol/PatrolAttentionWorkbench.tsx +++ b/frontend-modern/src/features/patrol/PatrolAttentionWorkbench.tsx @@ -64,6 +64,7 @@ import type { EvidenceEnvelope } from '@/types/operationalTrust'; import type { ActionDetailResponse } from '@/types/actionAudit'; import { getAlertResourceIncidentAcknowledgedByLabel } from '@/utils/alertIncidentPresentation'; import { formatRelativeTime } from '@/utils/format'; +import { useRelativeTimeNow } from '@/utils/relativeTimeClock'; import { copyToClipboard } from '@/utils/clipboard'; import type { PatrolAutonomyLevel } from '@/api/patrol'; import { @@ -1249,6 +1250,9 @@ function AttentionLifecycleControls(props: { onSuppress: (itemId: string, reason: string, expiresAt: string) => Promise; onUnsuppress: (itemId: string) => Promise; }) { + // The acknowledgement time never changes while the detail stays open, so its + // age reads the shared clock. + const now = useRelativeTimeNow(); const [showSuppression, setShowSuppression] = createSignal(false); const [reason, setReason] = createSignal(''); const [durationMs, setDurationMs] = createSignal(SUPPRESSION_DURATIONS[1].value); @@ -1276,7 +1280,7 @@ function AttentionLifecycleControls(props: { {(acknowledgement) => (

{getAlertResourceIncidentAcknowledgedByLabel(acknowledgement().by)}{' '} - {formatRelativeTime(acknowledgement().at, { compact: true })}. + {formatRelativeTime(acknowledgement().at, { compact: true, now: now() })}.

)} @@ -1415,6 +1419,9 @@ function AttentionLifecycleControls(props: { } function AttentionTimelineList(props: { timeline: AttentionItemDetail['timeline'] }) { + // The open detail is read once per selection and its transitions never + // change, so their ages read the shared clock. + const now = useRelativeTimeNow(); return (
    @@ -1424,7 +1431,7 @@ function AttentionTimelineList(props: { timeline: AttentionItemDetail['timeline' {formatLabel(transition.from)} to {formatLabel(transition.to)}

    - {formatRelativeTime(transition.at, { compact: true })} ·{' '} + {formatRelativeTime(transition.at, { compact: true, now: now() })} ·{' '} {formatLabel(transition.cause)}

    @@ -1748,6 +1755,9 @@ function DetailSection(props: { title: string; children: import('solid-js').JSX. } function EvidenceObservation(props: { evidence: EvidenceEnvelope }) { + // An evidence record's observation time is fixed, so its age reads the + // shared clock while the detail stays open. + const now = useRelativeTimeNow(); return (
  1. @@ -1771,7 +1781,7 @@ function EvidenceObservation(props: { evidence: EvidenceEnvelope }) {

    {props.evidence.source.collector} · observed{' '} - {formatRelativeTime(props.evidence.observedAt, { compact: true })} + {formatRelativeTime(props.evidence.observedAt, { compact: true, now: now() })}

    {(reason) =>

    {reason()}

    } diff --git a/frontend-modern/src/features/patrol/PatrolIntelligenceBanners.tsx b/frontend-modern/src/features/patrol/PatrolIntelligenceBanners.tsx index 059d311f05..d491135fb1 100644 --- a/frontend-modern/src/features/patrol/PatrolIntelligenceBanners.tsx +++ b/frontend-modern/src/features/patrol/PatrolIntelligenceBanners.tsx @@ -11,6 +11,7 @@ import { presentationPolicyHidesUpgradePrompts, } from '@/stores/sessionPresentationPolicy'; import { formatRelativeTime } from '@/utils/format'; +import { useRelativeTimeNow } from '@/utils/relativeTimeClock'; import { getAIProviderDisplayName } from '@/utils/aiProviderPresentation'; import { getPatrolSetupAction } from '@/utils/patrolRuntimeActions'; import { getPatrolAutonomyAvailabilityPresentation } from './patrolAutonomyAvailability'; @@ -30,6 +31,9 @@ function modelIdWithoutProviderPrefix(modelId: string | undefined): string { export function PatrolIntelligenceBanners(props: { state: PatrolIntelligenceState }) { const state = props.state; + // Patrol status re-reads keep the same blocked time while Patrol stays + // paused, so the Blocked age reads the shared clock. + const now = useRelativeTimeNow(); const setupAction = createMemo(() => getPatrolSetupAction( resolvePatrolBlockedActionCause(state.blockedCause(), state.patrolReadiness()?.cause), @@ -267,7 +271,7 @@ export function PatrolIntelligenceBanners(props: { state: PatrolIntelligenceStat

    {state.blockedReason()}

    - Blocked {formatRelativeTime(state.blockedAt(), { compact: true })} + Blocked {formatRelativeTime(state.blockedAt(), { compact: true, now: now() })}

    diff --git a/frontend-modern/src/features/patrol/PatrolIntelligenceWorkspace.tsx b/frontend-modern/src/features/patrol/PatrolIntelligenceWorkspace.tsx index 1bd38e07c6..b3c70a6068 100644 --- a/frontend-modern/src/features/patrol/PatrolIntelligenceWorkspace.tsx +++ b/frontend-modern/src/features/patrol/PatrolIntelligenceWorkspace.tsx @@ -12,6 +12,7 @@ import { isPatrolRuntimeFinding, } from '@/utils/aiFindingPresentation'; import { formatRelativeTime } from '@/utils/format'; +import { useRelativeTimeNow } from '@/utils/relativeTimeClock'; import { formatTriggerReason } from '@/utils/patrolFormat'; import { getPatrolRunRecordSummaryPresentation, @@ -51,6 +52,9 @@ export function PatrolIntelligenceWorkspace(props: { findingResourceId?: string; }) { const state = props.state; + // The selected run stays as picked when run history is re-read, so its start + // age reads the shared clock instead of freezing at selection. + const now = useRelativeTimeNow(); const queueDisplayGroups = createMemo(() => buildPatrolFindingDisplayGroups(state.findingsTabBadgeFindings()), ); @@ -207,7 +211,7 @@ export function PatrolIntelligenceWorkspace(props: {
    - Patrol run {formatRelativeTime(run().started_at, { compact: true })} + Patrol run {formatRelativeTime(run().started_at, { compact: true, now: now() })} {formatTriggerReason(run().trigger_reason)} diff --git a/frontend-modern/src/features/patrol/__tests__/PatrolAttentionWorkbench.test.tsx b/frontend-modern/src/features/patrol/__tests__/PatrolAttentionWorkbench.test.tsx index 3c5ff2b423..d0b5ac3b27 100644 --- a/frontend-modern/src/features/patrol/__tests__/PatrolAttentionWorkbench.test.tsx +++ b/frontend-modern/src/features/patrol/__tests__/PatrolAttentionWorkbench.test.tsx @@ -7,6 +7,7 @@ import type { AttentionListResponse, AttentionSummary, } from '@/api/patrolAttention'; +import { RELATIVE_TIME_TICK_MS } from '@/utils/relativeTimeClock'; const apiMocks = vi.hoisted(() => ({ getList: vi.fn(), @@ -479,6 +480,45 @@ describe('PatrolAttentionWorkbench', () => { expect(window.location.search).toBe('?attention=record-1'); }); + it('keeps the open detail transition, evidence and acknowledgement ages moving', async () => { + vi.useFakeTimers({ + toFake: ['Date', 'setInterval', 'clearInterval'], + now: Date.parse('2026-07-19T08:10:00Z'), + }); + const active = item({ state: 'acknowledged' }); + const opened = detail(active); + opened.timeline[0].at = '2026-07-19T08:03:00Z'; + opened.evidence[0].observedAt = '2026-07-19T08:05:00Z'; + opened.operationalRecord.acknowledgement = { by: 'admin', at: '2026-07-19T08:07:00Z' }; + apiMocks.getList.mockResolvedValue( + listResponse([active], summary({ activeCount: 1, acknowledgedCount: 1, calm: false })), + ); + apiMocks.getDetail.mockResolvedValue(opened); + + try { + renderWorkbench(); + fireEvent.click( + await screen.findByRole('button', { name: 'Open Database VM · Disk pressure' }), + ); + const detailRegion = await screen.findByRole('complementary', { + name: 'Database VM · Disk pressure', + }); + await waitFor(() => expect(detailRegion).toHaveTextContent('7m ago')); + expect(detailRegion).toHaveTextContent('5m ago'); + expect(detailRegion).toHaveTextContent('3m ago'); + + // The detail is not re-read: its records never change and only the + // clock moves. + vi.advanceTimersByTime(2 * 60 * 2 * RELATIVE_TIME_TICK_MS); + + expect(detailRegion).not.toHaveTextContent(/[357]m ago/); + expect(detailRegion.textContent?.match(/2h ago/g)?.length).toBeGreaterThanOrEqual(3); + expect(apiMocks.getDetail).toHaveBeenCalledTimes(1); + } finally { + vi.useRealTimers(); + } + }); + it('keeps the detail header and queue controls pinned while a long decision scrolls', async () => { const active = item(); apiMocks.getList.mockResolvedValue( diff --git a/frontend-modern/src/features/patrol/__tests__/PatrolRunBanners.relativeAge.test.tsx b/frontend-modern/src/features/patrol/__tests__/PatrolRunBanners.relativeAge.test.tsx new file mode 100644 index 0000000000..bb50875020 --- /dev/null +++ b/frontend-modern/src/features/patrol/__tests__/PatrolRunBanners.relativeAge.test.tsx @@ -0,0 +1,96 @@ +import { cleanup, render, screen } from '@solidjs/testing-library'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import type { PatrolRunRecord } from '@/api/patrol'; +import { RELATIVE_TIME_TICK_MS } from '@/utils/relativeTimeClock'; +import { PatrolIntelligenceBanners } from '../PatrolIntelligenceBanners'; +import { PatrolIntelligenceWorkspace } from '../PatrolIntelligenceWorkspace'; +import type { PatrolIntelligenceState } from '../usePatrolIntelligenceState'; + +vi.mock('@/components/AI/FindingsPanel', () => ({ + FindingsPanel: () =>
    , +})); + +vi.mock('@/components/patrol/RunHistoryPanel', () => ({ + RunHistoryPanel: () =>
    , +})); + +// Both surfaces read many accessors; the ones a test does not set answer +// with nothing, and list-valued ones with an empty list. +const LIST_ACCESSORS = new Set([ + 'findingsTabBadgeFindings', + 'displayRunHistory', + 'selectedRunFindingIds', + 'selectedRunScopeResourceIds', +]); +const stubState = (overrides: Record): PatrolIntelligenceState => + new Proxy(overrides, { + get(target, key: string) { + if (key in target) return target[key]; + if (key === 'patrolStream') { + return new Proxy({}, { get: () => () => undefined }); + } + if (key === 'patrolRunHistory') return { value: () => [], refetch: vi.fn() }; + return LIST_ACCESSORS.has(key) ? () => [] : () => undefined; + }, + }) as unknown as PatrolIntelligenceState; + +afterEach(() => { + cleanup(); + vi.useRealTimers(); +}); + +describe('Patrol run and pause banners relative ages', () => { + it('keeps the Patrol paused Blocked age moving while Patrol stays paused', () => { + vi.useFakeTimers({ + toFake: ['Date', 'setInterval', 'clearInterval'], + now: Date.parse('2026-07-19T08:05:00Z'), + }); + + render(() => ( + true, + blockedReason: () => 'The monthly AI budget is used up.', + blockedAt: () => '2026-07-19T08:00:00Z', + })} + /> + )); + + expect(screen.getByText('Blocked 5m ago')).toBeInTheDocument(); + + // Each status re-read returns the same blocked time: only the clock moves. + vi.advanceTimersByTime(3 * 60 * 2 * RELATIVE_TIME_TICK_MS); + + expect(screen.getByText('Blocked 3h ago')).toBeInTheDocument(); + }); + + it('keeps the selected Patrol run start age moving while the run stays selected', () => { + vi.useFakeTimers({ + toFake: ['Date', 'setInterval', 'clearInterval'], + now: Date.parse('2026-07-19T08:05:00Z'), + }); + const run = { + id: 'run-1', + started_at: '2026-07-19T08:00:00Z', + trigger_reason: 'scheduled', + status: 'healthy', + finding_ids: [], + } as unknown as PatrolRunRecord; + + render(() => ( + 'findings', + selectedRun: () => run, + })} + /> + )); + + expect(screen.getByText('Patrol run 5m ago')).toBeInTheDocument(); + + vi.advanceTimersByTime(3 * 60 * 2 * RELATIVE_TIME_TICK_MS); + + expect(screen.getByText('Patrol run 3h ago')).toBeInTheDocument(); + }); +}); From 32ef1d366d38fa666429200dce5a19b53bb16a4d Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Tue, 6 Oct 2026 17:45:19 +0100 Subject: [PATCH 02/12] Keep watchdog Refresh working when a status read hangs The background re-read added in the previous commit used one in-flight flag for both background and manual reads, and the status read has no timeout. A background read that never settled therefore made every later poll and every Refresh return at once, so Refresh stayed enabled but did nothing while the panel kept an old status. A read that finally answered late could also overwrite a newer one. Each read now gets a sequence number and only the newest one applies. Refresh always reads. A background read waits only while the newest read is younger than one poll interval, so a hung read is retried on the next poll, and its late answer is discarded. Nothing applies after the panel unmounts. --- .../v6/internal/subsystems/alerts.md | 6 ++- .../subsystems/frontend-primitives.md | 4 +- .../e24ba99586e7ea96.json | 36 +++++++++++++ .../alerts/AlertDeadManDestinationSection.tsx | 39 ++++++++++---- .../AlertDeadManDestinationSection.test.tsx | 52 +++++++++++++++++++ 5 files changed, 126 insertions(+), 11 deletions(-) create mode 100644 frontend-modern/browser-verification/e24ba99586e7ea96.json diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index 2cf5789a5a..d580a5e488 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -3048,7 +3048,11 @@ seconds. A moving age over a status read once at mount would claim the heartbeat stopped while Pulse kept pinging every minute, and a status that is never re-read kept its read-time age through a real stall, which is the one case this panel exists to show. A background re-read leaves the Refresh control -alone, and a failed one keeps the last status and marks it unavailable. +alone, and a failed one keeps the last status and marks it unavailable. The +status read has no timeout, so reads carry a sequence number and only the +newest one applies: a read that never settles cannot hold the panel on an old +status or swallow a later Refresh, the next background read goes out once it has +been pending a full interval, and a late answer from it is discarded. The delivery log's relative hover time on each row reads the same clock: a logged attempt's time never changes, so its age is correct over a log that is only re-read on demand. diff --git a/docs/release-control/v6/internal/subsystems/frontend-primitives.md b/docs/release-control/v6/internal/subsystems/frontend-primitives.md index efe7ea8740..35397fac76 100644 --- a/docs/release-control/v6/internal/subsystems/frontend-primitives.md +++ b/docs/release-control/v6/internal/subsystems/frontend-primitives.md @@ -5218,7 +5218,9 @@ not re-read stays the age at read time, because a moving age over a snapshot that never refreshes claims the reading stopped when it may not have. Such a surface either re-reads the snapshot in the background, as Proxmox replication and the external watchdog panel do, or keeps the read-time age, as the Patrol -attention detail does for Last seen. An immutable event time (when a delivery +attention detail does for Last seen. A background re-read applies only its +newest answer, so a read that never settles cannot pin the surface to an older +snapshot while its ages keep moving. An immutable event time (when a delivery was attempted, a transition happened, a policy was set) ages correctly over any snapshot and reads the clock. Read-only metadata badges follow the same primitive-owned shell rule. diff --git a/frontend-modern/browser-verification/e24ba99586e7ea96.json b/frontend-modern/browser-verification/e24ba99586e7ea96.json new file mode 100644 index 0000000000..9b91d5a3cd --- /dev/null +++ b/frontend-modern/browser-verification/e24ba99586e7ea96.json @@ -0,0 +1,36 @@ +{ + "version": 1, + "base_sha": "278c403d9674cb2eb2066707637e88770636658b", + "verified_at": "2026-10-06T16:44:58Z", + "result": "passed", + "changed_paths": [ + "frontend-modern/src/features/alerts/AlertDeadManDestinationSection.tsx" + ], + "content_sha256": { + "frontend-modern/src/features/alerts/AlertDeadManDestinationSection.tsx": "8df8585fbe2838b19227a80af8304cdfdd022b31f73f9deada8cb235bc73d8a2" + }, + "routes": [ + "/alerts/destinations" + ], + "viewports": [ + { + "width": 1440, + "height": 900 + }, + { + "width": 390, + "height": 844 + } + ], + "states": [ + "Private mock stack (backend :7731 built from this clone, Vite :5251 serving this branch) with Playwright page.clock installed before load and /api/alerts/deadman/status intercepted: the mount read answers a healthy status 10 minutes old, the first background read hangs until released and then answers a status 2 hours old, every later read answers a fresh status", + "After one 31-second poll the hung background read is pending; Refresh still issues a third read and applies it: Last success 10 mins ago -> 34s ago, Refresh enabled", + "Releasing the hung read's 2-hour-old answer leaves Last success at 34s ago (stale answer discarded), and the next poll issues a fourth read", + "Control with this file swapped back to the previous commit under the running Vite: Refresh did nothing (no third read, still 10 mins ago) and the hung read's stale answer won (2 hours ago)", + "No page errors at either viewport" + ], + "interactions": [ + "Loaded /alerts/destinations at 1440x900 and 390x844, ran the page clock 31 seconds, clicked Refresh, released the hung read, ran another 30 seconds", + "Repeated with the previous file as the control" + ] +} diff --git a/frontend-modern/src/features/alerts/AlertDeadManDestinationSection.tsx b/frontend-modern/src/features/alerts/AlertDeadManDestinationSection.tsx index b6e5f53ee1..fb916c129a 100644 --- a/frontend-modern/src/features/alerts/AlertDeadManDestinationSection.tsx +++ b/frontend-modern/src/features/alerts/AlertDeadManDestinationSection.tsx @@ -58,22 +58,43 @@ export function AlertDeadManDestinationSection(props: AlertDeadManDestinationSec const [unavailable, setUnavailable] = createSignal(false); const [showUrl, setShowUrl] = createSignal(false); - let backgroundLoad = false; + // The status read has no timeout, so every read gets a sequence number and + // only the newest one applies: a read that never settles cannot hold the + // panel on an old status or swallow a later Refresh. A background read + // waits while the newest read is younger than one poll interval and leaves + // the Refresh button alone; a failed read keeps the last status and marks it + // unavailable. Nothing applies after the panel unmounts. + let latestRequest = 0; + let pendingSince: number | undefined; + let disposed = false; + onCleanup(() => { + disposed = true; + }); const loadStatus = async (options: { background?: boolean } = {}) => { - if (loading() || backgroundLoad) return; - // A background re-read leaves the Refresh button alone; a failed one keeps - // the last status and marks it unavailable. - if (options.background) backgroundLoad = true; - else setLoading(true); + if (options.background) { + if (pendingSince !== undefined && Date.now() - pendingSince < DEAD_MAN_STATUS_POLL_MS) { + return; + } + } else if (loading()) { + return; + } + const request = ++latestRequest; + pendingSince = Date.now(); + if (!options.background) setLoading(true); try { - setStatus(await AlertsAPI.getDeadManStatus()); + const next = await AlertsAPI.getDeadManStatus(); + if (disposed || request !== latestRequest) return; + setStatus(next); setUnavailable(false); } catch (error) { + if (disposed || request !== latestRequest) return; logger.error('Failed to load external watchdog status', error); setUnavailable(true); } finally { - if (options.background) backgroundLoad = false; - else setLoading(false); + if (!disposed && request === latestRequest) { + pendingSince = undefined; + setLoading(false); + } } }; diff --git a/frontend-modern/src/features/alerts/__tests__/AlertDeadManDestinationSection.test.tsx b/frontend-modern/src/features/alerts/__tests__/AlertDeadManDestinationSection.test.tsx index 17ba5f8165..8cc27ad159 100644 --- a/frontend-modern/src/features/alerts/__tests__/AlertDeadManDestinationSection.test.tsx +++ b/frontend-modern/src/features/alerts/__tests__/AlertDeadManDestinationSection.test.tsx @@ -154,4 +154,56 @@ describe('AlertDeadManDestinationSection', () => { vi.useRealTimers(); } }); + + it('keeps Refresh working and ignores a stale answer when a background read hangs', async () => { + const start = Date.parse('2026-08-27T12:00:30Z'); + vi.useFakeTimers({ toFake: ['Date', 'setInterval', 'clearInterval'], now: start }); + const base = await AlertsAPI.getDeadManStatus(); + vi.mocked(AlertsAPI.getDeadManStatus).mockClear(); + let releaseHungRead: (value: typeof base) => void = () => undefined; + vi.mocked(AlertsAPI.getDeadManStatus) + .mockResolvedValueOnce(base) + .mockImplementationOnce( + () => + new Promise((resolve) => { + releaseHungRead = resolve; + }), + ) + .mockResolvedValue({ ...base, lastSuccessAt: '2026-08-27T12:00:55Z' }); + const [pingUrl, setPingUrl] = createSignal('***REDACTED***'); + + try { + render(() => ( + + )); + const lastSuccess = () => screen.getByText('Last success').nextElementSibling; + await waitFor(() => expect(lastSuccess()).toHaveTextContent('30s ago')); + + // The first background read never settles. + vi.advanceTimersByTime(DEAD_MAN_STATUS_POLL_MS); + await waitFor(() => expect(AlertsAPI.getDeadManStatus).toHaveBeenCalledTimes(2)); + + // Refresh still reads and applies the newer status. + fireEvent.click(screen.getByRole('button', { name: 'Refresh' })); + await waitFor(() => expect(AlertsAPI.getDeadManStatus).toHaveBeenCalledTimes(3)); + await waitFor(() => expect(lastSuccess()).toHaveTextContent('5s ago')); + expect(screen.getByRole('button', { name: 'Refresh' })).not.toBeDisabled(); + + // The hung read finally answers with the older status; it must not win. + releaseHungRead(base); + await Promise.resolve(); + await Promise.resolve(); + expect(lastSuccess()).toHaveTextContent('5s ago'); + + // Background reads keep going after the hang. + vi.advanceTimersByTime(DEAD_MAN_STATUS_POLL_MS); + await waitFor(() => expect(AlertsAPI.getDeadManStatus).toHaveBeenCalledTimes(4)); + } finally { + vi.useRealTimers(); + } + }); }); From 337e908b3577351ece5c4ffdf51ff17380e09b4e Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Tue, 6 Oct 2026 20:21:23 +0100 Subject: [PATCH 03/12] Record the browser pass for the combined Patrol attention detail Merging main brought the moved-alert presentation from #2591 into PatrolAttentionWorkbench.tsx alongside this branch's relative-time clock for the open attention detail. Neither commit's receipt covers the combined file, so this records the browser pass on the merged tree. --- .../f996b4691de6f0ce.json | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 frontend-modern/browser-verification/f996b4691de6f0ce.json diff --git a/frontend-modern/browser-verification/f996b4691de6f0ce.json b/frontend-modern/browser-verification/f996b4691de6f0ce.json new file mode 100644 index 0000000000..e1b524165d --- /dev/null +++ b/frontend-modern/browser-verification/f996b4691de6f0ce.json @@ -0,0 +1,34 @@ +{ + "version": 1, + "base_sha": "104af6eb6e6894055bfff2b6dc8cc9e046920735", + "verified_at": "2026-10-06T19:21:15Z", + "result": "passed", + "changed_paths": [ + "frontend-modern/src/features/patrol/PatrolAttentionWorkbench.tsx" + ], + "content_sha256": { + "frontend-modern/src/features/patrol/PatrolAttentionWorkbench.tsx": "a5666d3229bf78cc04eb16dd85c1b13ec3617e74d938ea08ac8d40a97bcf7ccb" + }, + "routes": [ + "/patrol" + ], + "viewports": [ + { + "width": 1440, + "height": 900 + }, + { + "width": 390, + "height": 844 + } + ], + "states": [ + "Combined content of PatrolAttentionWorkbench.tsx after merging origin/main (with #2591, moved-alert presentation) into this branch (relative-time clock for the open attention detail)", + "Private mock stack (backend :7731, Vite :5251 serving the merged tree) with Playwright page.clock installed before load; /ws data frames held with a data-free keep-alive per fake minute (one live socket), GETs replayed, collection refetches stalled", + "Patrol inbox: opened the first attention item; injected acknowledgement, timeline and evidence times 3, 5 and 7 minutes before load read 'Acknowledged by admin 3m ago' and 3m/5m/7m, then 13m/15m/17m after ten held minutes, at 1440 and 390", + "No page errors at either viewport; Patrol, Alerts and run history suites (106 files) pass on the merged tree" + ], + "interactions": [ + "Loaded /patrol, opened the first attention detail, held the stream and advanced the page clock ten one-minute steps, at both viewports" + ] +} From b81f0478d3d779f8c6e2be856f1e2b99acfc6437 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Tue, 6 Oct 2026 20:43:52 +0100 Subject: [PATCH 04/12] Apply watchdog reads in order and free a stalled Refresh The previous fix applied only the newest read. When every read took longer than the 30-second poll, each poll started a newer read before the last one answered, so every answer was discarded and the panel kept its old status (or none) indefinitely. A Refresh superseded by a background read that also hung left the button disabled for good. An answer now applies whenever it is newer than the last one applied, so slow reads land in order and an older answer never overwrites a newer one. Only the newest read's failure marks the status unavailable. A Refresh still pending at the first poll a full interval later frees the button. Both age checks allow one second of slack: in the browser the next tick can land a few milliseconds short of a full interval after the read the previous tick started, which skipped every other poll. --- .../v6/internal/subsystems/alerts.md | 10 +- .../subsystems/frontend-primitives.md | 7 +- .../0b7a94ad4faa1eb7.json | 38 ++++++++ .../alerts/AlertDeadManDestinationSection.tsx | 52 ++++++---- .../AlertDeadManDestinationSection.test.tsx | 94 +++++++++++++++++++ 5 files changed, 178 insertions(+), 23 deletions(-) create mode 100644 frontend-modern/browser-verification/0b7a94ad4faa1eb7.json diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index 4e1d591548..ca02ea2373 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -3174,10 +3174,12 @@ heartbeat stopped while Pulse kept pinging every minute, and a status that is never re-read kept its read-time age through a real stall, which is the one case this panel exists to show. A background re-read leaves the Refresh control alone, and a failed one keeps the last status and marks it unavailable. The -status read has no timeout, so reads carry a sequence number and only the -newest one applies: a read that never settles cannot hold the panel on an old -status or swallow a later Refresh, the next background read goes out once it has -been pending a full interval, and a late answer from it is discarded. +status read has no timeout, so reads carry a sequence number and an answer +applies only when it is newer than the last one applied: a slow or hung read can +neither overwrite a newer status nor block later reads, reads slower than the +poll still land in order, the next background read goes out at the first poll +once the newest has been pending a full interval, and a Refresh still pending at +such a poll frees the button (within two intervals of the click). The delivery log's relative hover time on each row reads the same clock: a logged attempt's time never changes, so its age is correct over a log that is only re-read on demand. diff --git a/docs/release-control/v6/internal/subsystems/frontend-primitives.md b/docs/release-control/v6/internal/subsystems/frontend-primitives.md index 9e3afd89f9..9ba9f2d3e8 100644 --- a/docs/release-control/v6/internal/subsystems/frontend-primitives.md +++ b/docs/release-control/v6/internal/subsystems/frontend-primitives.md @@ -5219,9 +5219,10 @@ not re-read stays the age at read time, because a moving age over a snapshot that never refreshes claims the reading stopped when it may not have. Such a surface either re-reads the snapshot in the background, as Proxmox replication and the external watchdog panel do, or keeps the read-time age, as the Patrol -attention detail does for Last seen. A background re-read applies only its -newest answer, so a read that never settles cannot pin the surface to an older -snapshot while its ages keep moving. An immutable event time (when a delivery +attention detail does for Last seen. A background re-read applies an answer +only when it is newer than the last one applied, so a slow or hung read can +neither pin the surface to an older snapshot nor overwrite a newer one while +its ages keep moving. An immutable event time (when a delivery was attempted, a transition happened, a policy was set) ages correctly over any snapshot and reads the clock. Read-only metadata badges follow the same primitive-owned shell rule. diff --git a/frontend-modern/browser-verification/0b7a94ad4faa1eb7.json b/frontend-modern/browser-verification/0b7a94ad4faa1eb7.json new file mode 100644 index 0000000000..0886986878 --- /dev/null +++ b/frontend-modern/browser-verification/0b7a94ad4faa1eb7.json @@ -0,0 +1,38 @@ +{ + "version": 1, + "base_sha": "337e908b3577351ece5c4ffdf51ff17380e09b4e", + "verified_at": "2026-10-06T19:43:25Z", + "result": "passed", + "changed_paths": [ + "frontend-modern/src/features/alerts/AlertDeadManDestinationSection.tsx" + ], + "content_sha256": { + "frontend-modern/src/features/alerts/AlertDeadManDestinationSection.tsx": "022a2f4b9412890a66420f18bda6105400c29c6deedf185dc102eccc063323d0" + }, + "routes": [ + "/alerts/destinations" + ], + "viewports": [ + { + "width": 1440, + "height": 900 + }, + { + "width": 390, + "height": 844 + } + ], + "states": [ + "Private mock stack (backend :7731, Vite :5251 serving this branch, merged with main 9aaac4d3e5) with Playwright page.clock installed before load and every /api/alerts/deadman/status read held until the script answers it", + "Reads slower than the poll: each poll starts a read while the previous is pending; answers applied in order (10 mins ago -> 3 mins ago -> 1 min ago), and an older read answered after a newer one did not overwrite it (stayed 1 min ago); every poll tick issued a read (no tick skipped to timer jitter)", + "A Refresh that never settles, followed by a background read that also never settles: Refresh showed Refreshing\u2026 disabled, then was enabled again at the first poll after it had been pending a full interval, and a second Refresh applied the fresh status", + "Earlier hang case still holds: with a hung background read, Refresh applies the fresh status and the hung read's late 2-hour-old answer is discarded", + "Control with the previous loader under the running Vite: slow reads never showed a status (every answer discarded) and the hung Refresh stayed disabled", + "No page errors at either viewport" + ], + "interactions": [ + "Loaded /alerts/destinations at 1440x900 and 390x844, ran the page clock in 30-second steps answering held reads in and out of order", + "Clicked Refresh with the read held, ran the clock 61 seconds, clicked Refresh again and answered it", + "Repeated with the previous loader as the control" + ] +} diff --git a/frontend-modern/src/features/alerts/AlertDeadManDestinationSection.tsx b/frontend-modern/src/features/alerts/AlertDeadManDestinationSection.tsx index fb916c129a..8450336946 100644 --- a/frontend-modern/src/features/alerts/AlertDeadManDestinationSection.tsx +++ b/frontend-modern/src/features/alerts/AlertDeadManDestinationSection.tsx @@ -18,6 +18,10 @@ const REDACTED_PING_URL = '***REDACTED***'; // Pulse pings the watchdog every minute, so a status read once at mount goes // stale while the panel stays open. export const DEAD_MAN_STATUS_POLL_MS = 30_000; +// A read pending this long at a poll tick counts as a full interval old. The +// slack absorbs timer jitter: the next tick can land a few milliseconds short +// of a full interval after the read the previous tick started. +const DEAD_MAN_STATUS_STALE_MS = DEAD_MAN_STATUS_POLL_MS - 1_000; const statusPresentation: Record = { disabled: { @@ -58,42 +62,58 @@ export function AlertDeadManDestinationSection(props: AlertDeadManDestinationSec const [unavailable, setUnavailable] = createSignal(false); const [showUrl, setShowUrl] = createSignal(false); - // The status read has no timeout, so every read gets a sequence number and - // only the newest one applies: a read that never settles cannot hold the - // panel on an old status or swallow a later Refresh. A background read - // waits while the newest read is younger than one poll interval and leaves - // the Refresh button alone; a failed read keeps the last status and marks it - // unavailable. Nothing applies after the panel unmounts. - let latestRequest = 0; + // The status read has no timeout. Every read gets a sequence number and an + // answer applies only when it is newer than the last one applied, so a slow + // or hung read can neither overwrite a newer status nor block later reads, + // and reads slower than the poll interval still land in order. A background + // read waits while the newest read is younger than one poll interval; a + // Refresh still pending at such a poll counts as stalled and frees the + // button. Only + // the newest read's failure marks the status unavailable, and nothing + // applies after the panel unmounts. + let latestStarted = 0; + let latestApplied = 0; let pendingSince: number | undefined; + let foregroundRequest = 0; + let foregroundSince = 0; let disposed = false; onCleanup(() => { disposed = true; }); const loadStatus = async (options: { background?: boolean } = {}) => { + const startedAt = Date.now(); if (options.background) { - if (pendingSince !== undefined && Date.now() - pendingSince < DEAD_MAN_STATUS_POLL_MS) { + if (loading() && startedAt - foregroundSince >= DEAD_MAN_STATUS_STALE_MS) { + setLoading(false); + } + if (pendingSince !== undefined && startedAt - pendingSince < DEAD_MAN_STATUS_STALE_MS) { return; } } else if (loading()) { return; } - const request = ++latestRequest; - pendingSince = Date.now(); - if (!options.background) setLoading(true); + const request = ++latestStarted; + pendingSince = startedAt; + if (!options.background) { + foregroundRequest = request; + foregroundSince = startedAt; + setLoading(true); + } try { const next = await AlertsAPI.getDeadManStatus(); - if (disposed || request !== latestRequest) return; + if (disposed || request < latestApplied) return; + latestApplied = request; setStatus(next); setUnavailable(false); + if (request >= foregroundRequest) setLoading(false); } catch (error) { - if (disposed || request !== latestRequest) return; + if (disposed || request !== latestStarted) return; logger.error('Failed to load external watchdog status', error); setUnavailable(true); } finally { - if (!disposed && request === latestRequest) { - pendingSince = undefined; - setLoading(false); + if (!disposed) { + if (request === latestStarted) pendingSince = undefined; + if (request === foregroundRequest) setLoading(false); } } }; diff --git a/frontend-modern/src/features/alerts/__tests__/AlertDeadManDestinationSection.test.tsx b/frontend-modern/src/features/alerts/__tests__/AlertDeadManDestinationSection.test.tsx index 8cc27ad159..1740cab1aa 100644 --- a/frontend-modern/src/features/alerts/__tests__/AlertDeadManDestinationSection.test.tsx +++ b/frontend-modern/src/features/alerts/__tests__/AlertDeadManDestinationSection.test.tsx @@ -206,4 +206,98 @@ describe('AlertDeadManDestinationSection', () => { vi.useRealTimers(); } }); + + it('applies reads slower than the poll in order and never lets an older answer win', async () => { + const start = Date.parse('2026-08-27T12:10:00Z'); + vi.useFakeTimers({ toFake: ['Date', 'setInterval', 'clearInterval'], now: start }); + const base = await AlertsAPI.getDeadManStatus(); + vi.mocked(AlertsAPI.getDeadManStatus).mockReset(); + const pending: Array<(value: typeof base) => void> = []; + vi.mocked(AlertsAPI.getDeadManStatus).mockImplementation( + () => + new Promise((resolve) => { + pending.push(resolve); + }), + ); + const at = (time: string) => ({ ...base, lastSuccessAt: time }); + const [pingUrl, setPingUrl] = createSignal('***REDACTED***'); + + try { + render(() => ( + + )); + await waitFor(() => expect(pending).toHaveLength(1)); + + // Every read takes longer than one poll interval. + vi.advanceTimersByTime(DEAD_MAN_STATUS_POLL_MS); + await waitFor(() => expect(pending).toHaveLength(2)); + pending[0](at('2026-08-27T12:00:00Z')); + const lastSuccess = () => screen.getByText('Last success').nextElementSibling; + await waitFor(() => expect(lastSuccess()).toHaveTextContent('10 mins ago')); + + vi.advanceTimersByTime(DEAD_MAN_STATUS_POLL_MS); + await waitFor(() => expect(pending).toHaveLength(3)); + pending[1](at('2026-08-27T12:10:00Z')); + await waitFor(() => expect(lastSuccess()).toHaveTextContent('1 min ago')); + + // A newer read answers before an older one: the older answer is dropped. + vi.advanceTimersByTime(DEAD_MAN_STATUS_POLL_MS); + await waitFor(() => expect(pending).toHaveLength(4)); + pending[3](at('2026-08-27T12:11:30Z')); + await waitFor(() => expect(lastSuccess()).toHaveTextContent('0s ago')); + pending[2](at('2026-08-27T11:00:00Z')); + await Promise.resolve(); + await Promise.resolve(); + expect(lastSuccess()).toHaveTextContent('0s ago'); + } finally { + vi.useRealTimers(); + } + }); + + it('frees a Refresh that hangs for a whole poll interval', async () => { + const start = Date.parse('2026-08-27T12:00:30Z'); + vi.useFakeTimers({ toFake: ['Date', 'setInterval', 'clearInterval'], now: start }); + const base = await AlertsAPI.getDeadManStatus(); + vi.mocked(AlertsAPI.getDeadManStatus).mockReset(); + vi.mocked(AlertsAPI.getDeadManStatus) + .mockResolvedValueOnce(base) + .mockImplementationOnce(() => new Promise(() => undefined)) + .mockImplementationOnce(() => new Promise(() => undefined)) + .mockResolvedValue({ ...base, lastSuccessAt: '2026-08-27T12:00:55Z' }); + const [pingUrl, setPingUrl] = createSignal('***REDACTED***'); + + try { + render(() => ( + + )); + const lastSuccess = () => screen.getByText('Last success').nextElementSibling; + await waitFor(() => expect(lastSuccess()).toHaveTextContent('30s ago')); + + fireEvent.click(screen.getByRole('button', { name: 'Refresh' })); + await waitFor(() => + expect(screen.getByRole('button', { name: 'Refreshing…' })).toBeDisabled(), + ); + + // The Refresh read never settles, and neither does the background read + // that the next poll starts. The button is usable again after one poll + // interval, and a second Refresh brings the newer status. + vi.advanceTimersByTime(DEAD_MAN_STATUS_POLL_MS); + await waitFor(() => expect(AlertsAPI.getDeadManStatus).toHaveBeenCalledTimes(3)); + await waitFor(() => + expect(screen.getByRole('button', { name: 'Refresh' })).not.toBeDisabled(), + ); + fireEvent.click(screen.getByRole('button', { name: 'Refresh' })); + await waitFor(() => expect(lastSuccess()).toHaveTextContent('5s ago')); + } finally { + vi.useRealTimers(); + } + }); }); From c933241cf23f17b8c798b3e72a2403c8c4077c47 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Tue, 6 Oct 2026 20:52:52 +0100 Subject: [PATCH 05/12] Let the newest settled watchdog read decide availability The previous commit ordered successful reads but still judged failures against the newest read started. When every read failed after longer than the poll, each failure was discarded and the panel kept saying Heartbeat healthy through an outage. An older read that succeeded late could also clear a newer failure and report a recovery that had not happened. A read's outcome now applies only when it is newer than the last read that settled, for successes and failures alike, and a Refresh is released by its own read or any newer one that settles. The contract notes describe the 29-second threshold and the release rules, and the frontend-primitives sentence is scoped to the watchdog, since other polled queries still apply only their newest read. --- .../v6/internal/subsystems/alerts.md | 16 ++++-- .../subsystems/frontend-primitives.md | 7 +-- .../d7b9435b593e965e.json | 38 +++++++++++++ .../alerts/AlertDeadManDestinationSection.tsx | 31 +++++----- .../AlertDeadManDestinationSection.test.tsx | 57 +++++++++++++++++++ 5 files changed, 124 insertions(+), 25 deletions(-) create mode 100644 frontend-modern/browser-verification/d7b9435b593e965e.json diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index ca02ea2373..56139aec7d 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -3174,12 +3174,16 @@ heartbeat stopped while Pulse kept pinging every minute, and a status that is never re-read kept its read-time age through a real stall, which is the one case this panel exists to show. A background re-read leaves the Refresh control alone, and a failed one keeps the last status and marks it unavailable. The -status read has no timeout, so reads carry a sequence number and an answer -applies only when it is newer than the last one applied: a slow or hung read can -neither overwrite a newer status nor block later reads, reads slower than the -poll still land in order, the next background read goes out at the first poll -once the newest has been pending a full interval, and a Refresh still pending at -such a poll frees the button (within two intervals of the click). +status read has no timeout, so reads carry a sequence number and a read's +outcome applies only when it is newer than the last read that settled: a +success shows its status and clears the unavailable mark, a failure sets it. A +slow or hung read can therefore neither overwrite a newer outcome nor block +later reads, and reads slower than the poll still land in order. While the +panel's poll runs, the next background read goes out at the first poll once the +newest read has been pending at least 29 seconds (one second of slack absorbs +timer jitter), and a Refresh is released by its own read, by any newer read that +settles, or by the first poll at which it has been pending that long, so a hung +Refresh frees within about two intervals of the click. The delivery log's relative hover time on each row reads the same clock: a logged attempt's time never changes, so its age is correct over a log that is only re-read on demand. diff --git a/docs/release-control/v6/internal/subsystems/frontend-primitives.md b/docs/release-control/v6/internal/subsystems/frontend-primitives.md index 9ba9f2d3e8..19606688a1 100644 --- a/docs/release-control/v6/internal/subsystems/frontend-primitives.md +++ b/docs/release-control/v6/internal/subsystems/frontend-primitives.md @@ -5219,10 +5219,9 @@ not re-read stays the age at read time, because a moving age over a snapshot that never refreshes claims the reading stopped when it may not have. Such a surface either re-reads the snapshot in the background, as Proxmox replication and the external watchdog panel do, or keeps the read-time age, as the Patrol -attention detail does for Last seen. A background re-read applies an answer -only when it is newer than the last one applied, so a slow or hung read can -neither pin the surface to an older snapshot nor overwrite a newer one while -its ages keep moving. An immutable event time (when a delivery +attention detail does for Last seen. The external watchdog panel orders its +re-reads by request, so a slow or hung read can neither pin it to an older +snapshot nor overwrite a newer outcome while its ages keep moving. An immutable event time (when a delivery was attempted, a transition happened, a policy was set) ages correctly over any snapshot and reads the clock. Read-only metadata badges follow the same primitive-owned shell rule. diff --git a/frontend-modern/browser-verification/d7b9435b593e965e.json b/frontend-modern/browser-verification/d7b9435b593e965e.json new file mode 100644 index 0000000000..23522425f0 --- /dev/null +++ b/frontend-modern/browser-verification/d7b9435b593e965e.json @@ -0,0 +1,38 @@ +{ + "version": 1, + "base_sha": "b81f0478d3d779f8c6e2be856f1e2b99acfc6437", + "verified_at": "2026-10-06T19:52:24Z", + "result": "passed", + "changed_paths": [ + "frontend-modern/src/features/alerts/AlertDeadManDestinationSection.tsx" + ], + "content_sha256": { + "frontend-modern/src/features/alerts/AlertDeadManDestinationSection.tsx": "ba80adc764c5b0e8055345654bf4fb0bf0aaeb17338ad0c0f5be820ff680dea5" + }, + "routes": [ + "/alerts/destinations" + ], + "viewports": [ + { + "width": 1440, + "height": 900 + }, + { + "width": 390, + "height": 844 + } + ], + "states": [ + "Private mock stack (backend :7731, Vite :5251 serving this branch) with Playwright page.clock installed before load and every /api/alerts/deadman/status read held until the script answers it, with a status or an HTTP 500", + "Failures slower than the poll: with two reads in flight the older one fails, and the badge changes from Heartbeat healthy to Status unavailable; the newest read then fails and an older read succeeds late, and the badge stays Status unavailable (no false recovery)", + "Successes slower than the poll still apply in order (10 mins ago -> 3 mins ago -> 1 min ago) and an older late success does not overwrite a newer one", + "A hung Refresh followed by a hung background read: the button is enabled again at the first poll after a full interval, and a second Refresh applies", + "Control with the previous loader under the running Vite: the slow failure was hidden and the badge stayed Heartbeat healthy", + "No page errors at either viewport" + ], + "interactions": [ + "Loaded /alerts/destinations at 1440x900 and 390x844, ran the page clock in 30-second steps answering held reads in and out of order with successes and HTTP 500s", + "Clicked Refresh with the read held, ran the clock 61 seconds, clicked Refresh again", + "Repeated with the previous loader as the control" + ] +} diff --git a/frontend-modern/src/features/alerts/AlertDeadManDestinationSection.tsx b/frontend-modern/src/features/alerts/AlertDeadManDestinationSection.tsx index 8450336946..7347d76fb2 100644 --- a/frontend-modern/src/features/alerts/AlertDeadManDestinationSection.tsx +++ b/frontend-modern/src/features/alerts/AlertDeadManDestinationSection.tsx @@ -62,17 +62,17 @@ export function AlertDeadManDestinationSection(props: AlertDeadManDestinationSec const [unavailable, setUnavailable] = createSignal(false); const [showUrl, setShowUrl] = createSignal(false); - // The status read has no timeout. Every read gets a sequence number and an - // answer applies only when it is newer than the last one applied, so a slow - // or hung read can neither overwrite a newer status nor block later reads, - // and reads slower than the poll interval still land in order. A background - // read waits while the newest read is younger than one poll interval; a - // Refresh still pending at such a poll counts as stalled and frees the - // button. Only - // the newest read's failure marks the status unavailable, and nothing - // applies after the panel unmounts. + // The status read has no timeout. Every read gets a sequence number, and a + // read's outcome applies only when it is newer than the last read that + // settled: a success shows its status and clears the unavailable mark, a + // failure sets it. A slow or hung read therefore cannot overwrite a newer + // outcome or block later reads, and reads slower than the poll interval + // still land in order. A background read waits while the newest read is + // younger than one poll interval (less a second of timer slack); a Refresh + // still pending at such a poll counts as stalled and frees the button. + // Nothing applies after the panel unmounts. let latestStarted = 0; - let latestApplied = 0; + let latestSettled = 0; let pendingSince: number | undefined; let foregroundRequest = 0; let foregroundSince = 0; @@ -101,19 +101,20 @@ export function AlertDeadManDestinationSection(props: AlertDeadManDestinationSec } try { const next = await AlertsAPI.getDeadManStatus(); - if (disposed || request < latestApplied) return; - latestApplied = request; + if (disposed || request < latestSettled) return; + latestSettled = request; setStatus(next); setUnavailable(false); - if (request >= foregroundRequest) setLoading(false); } catch (error) { - if (disposed || request !== latestStarted) return; + if (disposed || request < latestSettled) return; + latestSettled = request; logger.error('Failed to load external watchdog status', error); setUnavailable(true); } finally { if (!disposed) { if (request === latestStarted) pendingSince = undefined; - if (request === foregroundRequest) setLoading(false); + // A Refresh is answered by its own read or by any newer one. + if (request >= foregroundRequest) setLoading(false); } } }; diff --git a/frontend-modern/src/features/alerts/__tests__/AlertDeadManDestinationSection.test.tsx b/frontend-modern/src/features/alerts/__tests__/AlertDeadManDestinationSection.test.tsx index 1740cab1aa..b82779126a 100644 --- a/frontend-modern/src/features/alerts/__tests__/AlertDeadManDestinationSection.test.tsx +++ b/frontend-modern/src/features/alerts/__tests__/AlertDeadManDestinationSection.test.tsx @@ -300,4 +300,61 @@ describe('AlertDeadManDestinationSection', () => { vi.useRealTimers(); } }); + + it('lets the newest settled read decide availability, even when reads outlast the poll', async () => { + const start = Date.parse('2026-08-27T12:10:00Z'); + vi.useFakeTimers({ toFake: ['Date', 'setInterval', 'clearInterval'], now: start }); + const base = await AlertsAPI.getDeadManStatus(); + vi.mocked(AlertsAPI.getDeadManStatus).mockReset(); + const pending: Array<{ + resolve: (value: typeof base) => void; + reject: (error: Error) => void; + }> = []; + vi.mocked(AlertsAPI.getDeadManStatus).mockImplementation( + () => + new Promise((resolve, reject) => { + pending.push({ resolve, reject }); + }), + ); + const [pingUrl, setPingUrl] = createSignal('***REDACTED***'); + const badge = () => screen.getByText(/^(Status unavailable|Heartbeat healthy|Not configured)$/); + const settle = async () => { + await Promise.resolve(); + await Promise.resolve(); + }; + + try { + render(() => ( + + )); + await waitFor(() => expect(pending).toHaveLength(1)); + pending[0].resolve(base); + await waitFor(() => expect(badge()).toHaveTextContent('Heartbeat healthy')); + + // Every read now fails, and each failure arrives after the next poll has + // already started a newer read. The failures still mark the status. + vi.advanceTimersByTime(DEAD_MAN_STATUS_POLL_MS); + await waitFor(() => expect(pending).toHaveLength(2)); + vi.advanceTimersByTime(DEAD_MAN_STATUS_POLL_MS); + await waitFor(() => expect(pending).toHaveLength(3)); + pending[1].reject(new Error('timeout')); + await waitFor(() => expect(badge()).toHaveTextContent('Status unavailable')); + + // The newest read fails; an older read then succeeds late. The older + // success must not report a recovery. + vi.advanceTimersByTime(DEAD_MAN_STATUS_POLL_MS); + await waitFor(() => expect(pending).toHaveLength(4)); + pending[3].reject(new Error('timeout')); + await settle(); + pending[2].resolve(base); + await settle(); + expect(badge()).toHaveTextContent('Status unavailable'); + } finally { + vi.useRealTimers(); + } + }); }); From 5feda4836c74939b50d05a13fc485152137b3e46 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 08:54:49 +0100 Subject: [PATCH 06/12] Lead open History rows with the live reading of held alerts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Alerts > History is a log of occurrences, but its open row is the one still in progress: it shows an Active badge and a running duration, so "Node temperature at 95.0°C" beside them reads as the reading now while the node sits at 78°C under its 80°C trigger. The Alerts card one tab away already says "78°C now, back under the 80°C alert level", so the two pages disagreed about the same alert. An active row for a threshold alert with a usable live status now leads with the same presentation summary. The desktop row keeps the clear rule and the last breach in its hover; the phone card shows them as muted lines because a phone has no hover. The row's description stays the recorded message, so search and the Assistant handoff still read the record, and closed rows and open rows without a live status are unchanged. The reading rechecks on the shared clock, read only for open rows, so it turns stale when evaluations stop without re-rendering every closed row on each tick. Refs #2068 --- .../v6/internal/subsystems/alerts.md | 13 ++ .../subsystems/frontend-primitives.md | 9 ++ .../9c0e53268f0d014f.json | 45 +++++++ .../alerts/AlertHistoryMobileList.tsx | 20 ++- .../alerts/AlertHistoryTableAlertRow.tsx | 10 +- .../__tests__/AlertHistoryMobileList.test.tsx | 58 +++++++++ .../AlertHistoryTableAlertRow.test.tsx | 122 ++++++++++++++++++ .../__tests__/alertHistoryModel.test.ts | 114 +++++++++++++++- .../src/features/alerts/alertHistoryModel.ts | 41 ++++++ .../pages/__tests__/Alerts.helpers.test.ts | 4 + 10 files changed, 432 insertions(+), 4 deletions(-) create mode 100644 frontend-modern/browser-verification/9c0e53268f0d014f.json create mode 100644 frontend-modern/src/features/alerts/__tests__/AlertHistoryTableAlertRow.test.tsx diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index e00fd188b7..6a811a308c 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -49,6 +49,19 @@ recorded message. Every disk alert on a guest carries the guest's name, so the presentation names the disk from `metadata.label`, as the per-disk message does. +### History leads its open row with the live reading — issue #2068 + +Alerts > History is a log of occurrences, but its open row is the one +occurrence still in progress: it already shows an Active badge and a running +duration, so a breach message beside them reads as the reading now. An active +history row for a threshold alert with a usable live status leads with the +presentation's summary; the desktop row carries the clear rule and last +breach in its hover, and the phone card shows them as muted lines because it +has no hover. `buildAlertHistoryItems` keeps `description` as the recorded +message, so search and the Assistant handoff still read the record, and +attaches the source alert as `liveAlert` only to active, non-system rows. +Closed rows, and open ones without a live status, keep their recorded message. + ### Retained guest observations are not alert evidence Guest memory marked `last-known`, `unavailable` or an unknown observation state diff --git a/docs/release-control/v6/internal/subsystems/frontend-primitives.md b/docs/release-control/v6/internal/subsystems/frontend-primitives.md index 53b15f10dd..feda622ea2 100644 --- a/docs/release-control/v6/internal/subsystems/frontend-primitives.md +++ b/docs/release-control/v6/internal/subsystems/frontend-primitives.md @@ -43,6 +43,15 @@ card's left column (status icon and text) keeps a 16rem floor and the text breaks long words, so the action buttons wrap instead of squeezing the reading to its longest word beside them. +### History rows read held alerts through the same helper — issue #2068 + +`AlertHistoryTableAlertRow` and `AlertHistoryMobileList` take their message +copy from `getAlertHistoryRowCopy` in `features/alerts/alertHistoryModel.ts`, +which wraps the same `metricAlertPresentation.ts` helper for an active row's +`liveAlert`. It reads the `useRelativeTimeNow` clock only for rows with a live +alert, so a reading that stops updating turns stale on screen while closed +rows never re-render on the tick. + ### Canonical drawer History preserves guest read provenance The shared resource drawer passes selected memory observation state/source/time diff --git a/frontend-modern/browser-verification/9c0e53268f0d014f.json b/frontend-modern/browser-verification/9c0e53268f0d014f.json new file mode 100644 index 0000000000..fb11cc38ea --- /dev/null +++ b/frontend-modern/browser-verification/9c0e53268f0d014f.json @@ -0,0 +1,45 @@ +{ + "version": 1, + "base_sha": "a570d9f335cf8cd13a9df00a123a52234ec04a23", + "verified_at": "2026-10-07T07:58:00Z", + "result": "passed", + "changed_paths": [ + "frontend-modern/src/features/alerts/AlertHistoryMobileList.tsx", + "frontend-modern/src/features/alerts/AlertHistoryTableAlertRow.tsx", + "frontend-modern/src/features/alerts/alertHistoryModel.ts" + ], + "content_sha256": { + "frontend-modern/src/features/alerts/AlertHistoryMobileList.tsx": "71cf89c4a4bba27a8efdf3332260d033273185359c45eb01a6dcf7317cf1b53f", + "frontend-modern/src/features/alerts/AlertHistoryTableAlertRow.tsx": "b2b4199c76b0ccee79fad658bf91cb44174d5dbe5491942e846dc196af06e608", + "frontend-modern/src/features/alerts/alertHistoryModel.ts": "5e81adf1ad8c42224f250fbf9dc5e9b13fbfa0ed92b96802bb1af56cd139cbd6" + }, + "routes": [ + "/alerts/history?q=pve2", + "/alerts/history?q=95.0" + ], + "viewports": [ + { + "width": 1440, + "height": 900 + }, + { + "width": 390, + "height": 844 + } + ], + "states": [ + "Private mock stack built from this tree (main bea0cec8b9 + alert-card follow-up #2637 + this change), with an uncommitted generator overlay pinning pve2 CPU temperature; node temperature alert level 80\u00b0C, clear level 75\u00b0C, 5 minute recovery delay", + "History desktop row, held at 78\u00b0C: \"Temperature 78\u00b0C now, back under the 80\u00b0C alert level\", hover adds \"Stays open until it reaches 75\u00b0C or lower and stays there for 5 minutes.\" and \"Last reading at or above 80\u00b0C: 95\u00b0C\"; the breach message \"Node temperature at 95.0\u00b0C\" is not shown; light and dark", + "History phone card at 390px, held: live summary, clear rule and last breach as muted lines under it, no horizontal scroll, light and dark", + "History desktop row and phone card, recovering at 72\u00b0C: \"Temperature 72\u00b0C now, recovering\" with \"Clears after 5 minutes at 75\u00b0C or lower, 1 minute so far.\"", + "Search q=95.0 finds the held pve2 row through its recorded message while the row reads 72\u00b0C", + "Active non-threshold row (powered off) and resolved/acknowledged rows keep their recorded messages; no console errors on either route", + "Earlier pass on the pre-merge content (identical row code; the phone card now also carries main's moved-close line for closed rows): breaching at 95\u00b0C, Fahrenheit preference (172\u00b0F now, 176\u00b0F alert level, 203\u00b0F breach), resolved alert leaving the list" + ], + "interactions": [ + "Walked pve2 temperature 95 -> 78 -> 72 via the control file", + "Read description cell text and hover titles (desktop) and card paragraphs (phone) with Playwright in each phase", + "Loaded the q=pve2 and q=95.0 search URLs and collected console errors", + "Captured light and dark screenshots at 1440x900 and 390x844" + ] +} diff --git a/frontend-modern/src/features/alerts/AlertHistoryMobileList.tsx b/frontend-modern/src/features/alerts/AlertHistoryMobileList.tsx index 0cffed81e0..beee22a902 100644 --- a/frontend-modern/src/features/alerts/AlertHistoryMobileList.tsx +++ b/frontend-modern/src/features/alerts/AlertHistoryMobileList.tsx @@ -1,5 +1,6 @@ import { Show, createMemo, createSignal, onCleanup, onMount } from 'solid-js'; import { aiChatStore } from '@/stores/aiChat'; +import { useRelativeTimeNow } from '@/utils/relativeTimeClock'; import { getAlertHistoryResourceTypeBadgeClass } from '@/utils/alertHistoryPresentation'; import { @@ -8,6 +9,7 @@ import { } from '@/utils/alertIncidentPresentation'; import { AlertHistoryItemActions } from './AlertHistoryItemActions'; +import { getAlertHistoryRowCopy } from './alertHistoryModel'; import { PlatformWindowedList } from '@/features/platformPage/PlatformWindowedList'; import { getGroupSummaryLabel } from './AlertHistoryTableGroupRow'; import { @@ -30,6 +32,9 @@ export function AlertHistoryMobileList(props: AlertHistoryMobileListProps) { const [investigation, setInvestigation] = createSignal( null, ); + // Open threshold alerts lead with their live reading, which turns stale when + // evaluations stop, so it rechecks on the shared clock. + const now = useRelativeTimeNow(); const mobileItems = createMemo(() => { const items: AlertHistoryMobileItem[] = []; @@ -199,6 +204,7 @@ export function AlertHistoryMobileList(props: AlertHistoryMobileListProps) { const alert = item.alert; const rowKey = () => props.state.getIncidentRowKey(alert); const status = () => getAlertHistoryStatusPresentation(alert.status); + const descriptionCopy = () => getAlertHistoryRowCopy(alert, now); return (
    -

    {alert.description}

    +

    + {descriptionCopy().text} +

    + {/* A phone has no hover, so what clears the alert and its last + breach stay visible under the live reading. */} + + {(detail) =>

    {detail()}

    } +     + + {(lastBreach) => ( +

    {lastBreach()}

    + )} +     {(detail) =>

    {detail()}

    }     diff --git a/frontend-modern/src/features/alerts/AlertHistoryTableAlertRow.tsx b/frontend-modern/src/features/alerts/AlertHistoryTableAlertRow.tsx index 13d1b3b797..b760d3ae24 100644 --- a/frontend-modern/src/features/alerts/AlertHistoryTableAlertRow.tsx +++ b/frontend-modern/src/features/alerts/AlertHistoryTableAlertRow.tsx @@ -8,8 +8,10 @@ import { getAlertHistoryStatusPresentation, getAlertIncidentLevelBadgeClass, } from '@/utils/alertIncidentPresentation'; +import { useRelativeTimeNow } from '@/utils/relativeTimeClock'; import { AlertHistoryItemActions } from './AlertHistoryItemActions'; +import { getAlertHistoryRowCopy } from './alertHistoryModel'; import { AlertResourceIncidentsPanel } from './AlertResourceIncidentsPanel'; import type { AlertHistoryState } from './useAlertHistoryState'; @@ -23,6 +25,10 @@ interface AlertHistoryTableAlertRowProps { export function AlertHistoryTableAlertRow(props: AlertHistoryTableAlertRowProps) { const rowKey = () => props.state.getIncidentRowKey(props.alert); const historyStatusPresentation = () => getAlertHistoryStatusPresentation(props.alert.status); + // An open threshold alert's live reading turns stale when evaluations stop, + // so it rechecks on the shared clock; the last breach moves to the hover. + const now = useRelativeTimeNow(); + const descriptionCopy = () => getAlertHistoryRowCopy(props.alert, now); return ( <> @@ -62,9 +68,9 @@ export function AlertHistoryTableAlertRow(props: AlertHistoryTableAlertRowProps) - {props.alert.description} + {descriptionCopy().text} ({ + InvestigateAlertButton: () => null, +})); + function createState() { const alert = { id: 'alert-1', @@ -138,6 +143,59 @@ describe('AlertHistoryMobileList', () => { expect(screen.getAllByRole('heading', { name: 'Resource incidents' })).toHaveLength(1); }); + it('leads an open threshold alert with its live reading and keeps the breach visible', () => { + const { state, setGroupedAlerts } = createState(); + const observedAt = new Date(Date.now() - 10_000).toISOString(); + setGroupedAlerts([ + { + label: 'Today (August 4th)', + fullLabel: 'Today, August 4th 2026', + alerts: [ + { + id: 'minipc::metric-threshold:temperature', + source: 'alert', + resourceId: 'node-1', + resourceName: 'minipc', + resourceType: 'node', + title: 'Temperature', + description: 'Node temperature at 95.0°C', + severity: 'warning', + status: 'active', + duration: '30m', + startTime: '2026-08-04T14:30:00.000Z', + node: 'minipc', + nodeDisplayName: 'minipc', + rawAlertType: 'temperature', + liveAlert: { + type: 'temperature', + value: 95, + lastSeen: new Date(Date.now() - 20 * 60 * 1000).toISOString(), + metricStatus: { + phase: 'latched', + value: 78, + unit: '°C', + observedAt, + trigger: 80, + recovery: 75, + recoveryDelaySeconds: 300, + }, + }, + } as never, + ], + }, + ]); + + render(() => ); + + expect(screen.getByText('Temperature 78°C now, back under the 80°C alert level')).toBeVisible(); + expect( + screen.getByText('Stays open until it reaches 75°C or lower and stays there for 5 minutes.'), + ).toBeVisible(); + // A phone has no hover, so the last breach stays on the card. + expect(screen.getByText(/^Last reading at or above 80°C: 95°C, /)).toBeVisible(); + expect(screen.queryByText('Node temperature at 95.0°C')).toBeNull(); + }); + it('keeps resource investigation scrolling outside the virtualized history row', async () => { const { state } = createState(); diff --git a/frontend-modern/src/features/alerts/__tests__/AlertHistoryTableAlertRow.test.tsx b/frontend-modern/src/features/alerts/__tests__/AlertHistoryTableAlertRow.test.tsx new file mode 100644 index 0000000000..03f55e75db --- /dev/null +++ b/frontend-modern/src/features/alerts/__tests__/AlertHistoryTableAlertRow.test.tsx @@ -0,0 +1,122 @@ +import { render, screen } from '@solidjs/testing-library'; +import { createSignal } from 'solid-js'; +import { createStore } from 'solid-js/store'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import type { Alert, MetricAlertStatus } from '@/types/api'; + +import { AlertHistoryTableAlertRow } from '../AlertHistoryTableAlertRow'; +import type { HistoryItem } from '../alertHistoryModel'; +import type { AlertHistoryState } from '../useAlertHistoryState'; + +// Open rows offer the Assistant investigation, which needs the router. +vi.mock('@/components/Alerts/InvestigateAlertButton', () => ({ + InvestigateAlertButton: () => null, +})); + +const NOW = Date.parse('2026-10-06T19:00:00Z'); + +function createState() { + const [expandedIncidents] = createSignal(new Set()); + const [resourceIncidentPanel] = createSignal(null); + return { + getIncidentRowKey: (alert: HistoryItem) => `${alert.id}-row`, + expandedIncidents, + resourceIncidentPanel, + formatAlertRowTime: () => '18:30', + formatAlertRowTimestamp: () => 'Tuesday, 6 October 2026 at 18:30:00', + } as unknown as AlertHistoryState; +} + +const historyItem = (overrides: Partial): HistoryItem => ({ + id: 'minipc::metric-threshold:temperature', + source: 'alert', + status: 'active', + startTime: '2026-10-06T18:30:00Z', + duration: '30m', + resourceName: 'minipc', + resourceType: 'Node', + resourceId: 'minipc', + node: 'minipc', + severity: 'warning', + title: 'Temperature', + rawAlertType: 'temperature', + description: 'Node temperature at 95.0°C', + ...overrides, +}); + +const renderRow = (alert: HistoryItem) => + render(() => ( + + + + +
    + )); + +describe('AlertHistoryTableAlertRow', () => { + beforeEach(() => { + vi.useFakeTimers(); + vi.setSystemTime(NOW); + }); + afterEach(() => { + vi.useRealTimers(); + }); + + // minipc on 2026-10-06: the node read 78°C under its 80°C trigger and the + // open row still said "Node temperature at 95.0°C". + it('leads an open threshold alert with the reading now and follows it live', () => { + const [liveAlert, setLiveAlert] = createStore< + Pick + >({ + type: 'temperature', + value: 95, + lastSeen: '2026-10-06T18:40:00Z', + metricStatus: { + phase: 'latched', + value: 78, + unit: '°C', + observedAt: new Date(NOW - 10_000).toISOString(), + trigger: 80, + recovery: 75, + recoveryDelaySeconds: 300, + }, + }); + renderRow(historyItem({ liveAlert })); + + const cell = screen.getByText('Temperature 78°C now, back under the 80°C alert level'); + expect(cell.getAttribute('title')?.split('\n')).toEqual([ + 'Temperature 78°C now, back under the 80°C alert level', + 'Stays open until it reaches 75°C or lower and stays there for 5 minutes.', + 'Last reading at or above 80°C: 95°C, 20 mins ago', + ]); + expect(screen.queryByText('Node temperature at 95.0°C')).toBeNull(); + + // The next evaluation reaches the clear level; the row follows it. + setLiveAlert('metricStatus', (status) => ({ + ...(status as MetricAlertStatus), + phase: 'recovering', + value: 72, + observedAt: new Date(NOW).toISOString(), + })); + expect(cell.textContent).toBe('Temperature 72°C now, recovering'); + + // Evaluations stop: on the shared clock the reading stops being "now". + vi.advanceTimersByTime(15 * 60 * 1000); + expect(cell.textContent).toMatch(/^Last reading: Temperature 72°C, /); + }); + + it('keeps the recorded message on a resolved row', () => { + renderRow( + historyItem({ + status: 'resolved', + endTime: '2026-10-06T18:50:00Z', + description: 'Resolved: Node temperature at 74.0°C', + }), + ); + expect(screen.getByText('Resolved: Node temperature at 74.0°C')).toHaveAttribute( + 'title', + 'Resolved: Node temperature at 74.0°C', + ); + }); +}); diff --git a/frontend-modern/src/features/alerts/__tests__/alertHistoryModel.test.ts b/frontend-modern/src/features/alerts/__tests__/alertHistoryModel.test.ts index 0a9bc19072..58aa21e381 100644 --- a/frontend-modern/src/features/alerts/__tests__/alertHistoryModel.test.ts +++ b/frontend-modern/src/features/alerts/__tests__/alertHistoryModel.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from 'vitest'; +import { describe, expect, it, vi } from 'vitest'; import type { Alert } from '@/types/api'; import type { Resource } from '@/types/resource'; @@ -11,6 +11,7 @@ import { buildSelectedBucketDetails, formatAlertHistoryDuration, getAlertBucketDurationLabel, + getAlertHistoryRowCopy, type HistoryItem, } from '../alertHistoryModel'; @@ -120,3 +121,114 @@ describe('alertHistoryModel', () => { expect(sorted.map((item) => item.id)).toEqual(['newest', 'alpha', 'mid', 'zeta']); }); }); + +// minipc on 2026-10-06: the node temperature alert opened at 95°C, the node +// then read 78°C under its 80°C trigger, and the History row for the open +// alert still said "Node temperature at 95.0°C". +describe('alert history rows for held threshold alerts', () => { + const NOW = Date.parse('2026-10-06T19:00:00Z'); + const heldAlert = (status?: Partial>): Alert => + ({ + id: 'homelab-minipc::metric-threshold:temperature', + type: 'temperature', + level: 'warning', + resourceId: 'homelab-minipc', + resourceName: 'minipc', + node: 'minipc', + instance: 'homelab', + message: 'Node temperature at 95.0°C', + value: 95, + threshold: 80, + startTime: '2026-10-06T18:30:00Z', + lastSeen: '2026-10-06T18:40:00Z', + acknowledged: false, + metricStatus: status + ? { + phase: 'latched', + value: 78, + unit: '°C', + observedAt: new Date(NOW - 10_000).toISOString(), + trigger: 80, + recovery: 75, + recoveryDelaySeconds: 300, + ...status, + } + : undefined, + }) as Alert; + const build = (activeAlerts: Record, alertHistory: Alert[] = []) => + buildAlertHistoryItems({ + activeAlerts, + alertHistory, + getResource: () => undefined, + allResources: [], + now: NOW, + }); + + it('leads the open row with the live reading and keeps the breach for hover', () => { + const alert = heldAlert({ phase: 'latched', value: 78 }); + const [item] = build({ [alert.id]: alert }); + + // The record itself is unchanged: search and the Assistant handoff read it. + expect(item.description).toBe('Node temperature at 95.0°C'); + const copy = getAlertHistoryRowCopy(item, () => NOW); + expect(copy.text).toBe('Temperature 78°C now, back under the 80°C alert level'); + expect(copy.detail).toBe( + 'Stays open until it reaches 75°C or lower and stays there for 5 minutes.', + ); + expect(copy.lastBreach).toMatch(/^Last reading at or above 80°C: 95°C, /); + expect(copy.title.split('\n')).toEqual([copy.text, copy.detail, copy.lastBreach]); + }); + + it('says a recovering open row is recovering, with its progress', () => { + const alert = heldAlert({ phase: 'recovering', value: 72, recoveryElapsedSeconds: 130 }); + const copy = getAlertHistoryRowCopy(build({ [alert.id]: alert })[0], () => NOW); + expect(copy.text).toBe('Temperature 72°C now, recovering'); + expect(copy.detail).toBe('Clears after 5 minutes at 75°C or lower, 2 minutes so far.'); + }); + + it('stops calling a reading "now" once evaluations stop', () => { + const alert = heldAlert({ phase: 'latched', value: 78 }); + const [item] = build({ [alert.id]: alert }); + const copy = getAlertHistoryRowCopy(item, () => NOW + 15 * 60 * 1000); + expect(copy.text).toMatch(/^Last reading: Temperature 78°C, /); + expect(copy.text).not.toContain('now'); + }); + + it('keeps the recorded message on closed rows and open rows without a live status', () => { + const open = heldAlert(); + const closed = { + ...heldAlert({ phase: 'latched', value: 78 }), + id: 'homelab-minipc::metric-threshold:cpu', + message: 'Resolved: Node temperature at 74.0°C', + }; + const clock = vi.fn(() => NOW); + const items = build({ [open.id]: open }, [closed]); + const openItem = items.find((item) => item.id === open.id)!; + const closedItem = items.find((item) => item.id === closed.id)!; + + expect(closedItem.status).toBe('resolved'); + expect(closedItem.liveAlert).toBeUndefined(); + expect(getAlertHistoryRowCopy(closedItem, clock)).toEqual({ + text: 'Resolved: Node temperature at 74.0°C', + title: 'Resolved: Node temperature at 74.0°C', + }); + // Closed rows never read the clock, so they do not re-render on its tick. + expect(clock).not.toHaveBeenCalled(); + expect(getAlertHistoryRowCopy(openItem, clock)).toEqual({ + text: 'Node temperature at 95.0°C', + title: 'Node temperature at 95.0°C', + }); + }); + + it('gives Pulse system alerts no live reading', () => { + const alert = { + ...heldAlert({ phase: 'latched', value: 78 }), + id: 'pulse-system::storage', + metadata: { systemAlert: true }, + } as Alert; + const [item] = build({ [alert.id]: alert }); + expect(item.systemAlert).toBe(true); + expect(item.liveAlert).toBeUndefined(); + expect(getAlertHistoryRowCopy(item, () => NOW).text).toBe('Node temperature at 95.0°C'); + }); +}); diff --git a/frontend-modern/src/features/alerts/alertHistoryModel.ts b/frontend-modern/src/features/alerts/alertHistoryModel.ts index 097a560da5..0be5a89f2b 100644 --- a/frontend-modern/src/features/alerts/alertHistoryModel.ts +++ b/frontend-modern/src/features/alerts/alertHistoryModel.ts @@ -7,6 +7,7 @@ import { import { isPulseSystemAlert } from '@/utils/alertScope'; import { alertTypeDisplayLabel, unifiedTypeToAlertDisplayType } from './helpers'; +import { getMetricAlertPresentation } from './metricAlertPresentation'; export const MS_PER_HOUR = 60 * 60 * 1000; @@ -33,6 +34,22 @@ export interface HistoryItem { closeDetail?: string; acknowledged?: boolean; systemAlert?: boolean; + /** + * The open alert behind an active row, so the row can describe a threshold + * alert held below its trigger from its live reading. Closed rows have none. + */ + liveAlert?: Pick; +} + +export interface AlertHistoryRowCopy { + /** What the row leads with. */ + text: string; + /** What clears an open threshold alert from here. */ + detail?: string; + /** The last reading that met the trigger, once the live one differs. */ + lastBreach?: string; + /** The full copy, for hover where the row truncates. */ + title: string; } export interface AlertTrendSeries { @@ -205,6 +222,7 @@ export function buildAlertHistoryItems({ rawAlertType: alert.type, description: alert.message, acknowledged: false, + ...(systemScoped ? {} : { liveAlert: alert }), }); }); @@ -251,6 +269,29 @@ export function buildAlertHistoryItems({ return items; } +// The open row is the one occurrence still in progress. A threshold alert can +// stay open below its trigger while its message keeps the last breach, so the +// row leads with the reading Pulse is evaluating now, as the Alerts card does. +// Closed rows, and open ones without a live status, keep the recorded message, +// which search and the Assistant handoff also read. The clock is read only for +// open rows, so closed rows do not re-render on every tick. +export function getAlertHistoryRowCopy( + item: Pick, + now: () => number = Date.now, +): AlertHistoryRowCopy { + const recorded = item.description ?? ''; + const presentation = item.liveAlert ? getMetricAlertPresentation(item.liveAlert, now()) : null; + if (!presentation) return { text: recorded, title: recorded }; + return { + text: presentation.summary, + detail: presentation.detail, + lastBreach: presentation.lastBreach, + title: [presentation.summary, presentation.detail, presentation.lastBreach] + .filter(Boolean) + .join('\n'), + }; +} + export function filterAlertHistoryItems( items: HistoryItem[], severityFilter: AlertSeverityFilter, diff --git a/frontend-modern/src/pages/__tests__/Alerts.helpers.test.ts b/frontend-modern/src/pages/__tests__/Alerts.helpers.test.ts index 31016b5670..ab56ce918e 100644 --- a/frontend-modern/src/pages/__tests__/Alerts.helpers.test.ts +++ b/frontend-modern/src/pages/__tests__/Alerts.helpers.test.ts @@ -549,6 +549,10 @@ describe('tab path helpers', () => { expect(alertHistoryTableAlertRowSource).toContain('export function AlertHistoryTableAlertRow'); expect(alertHistoryTableAlertRowSource).toContain('IncidentTimelinePanel'); expect(alertHistoryTableAlertRowSource).toContain('AlertHistoryItemActions'); + // An open threshold alert's row leads with its live reading, not the + // breach message it opened with (#2068). + expect(alertHistoryTableAlertRowSource).toContain('getAlertHistoryRowCopy(props.alert, now)'); + expect(alertHistoryTableAlertRowSource).not.toContain('{props.alert.description}'); expect(alertHistoryItemActionsSource).toContain('InvestigateAlertButton'); expect(alertHistoryAdministrationCardSource).toContain( 'export function AlertHistoryAdministrationCard', From 8d00d54740c36d62b9505bd674d5f4a150d8096e Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 12:13:49 +0100 Subject: [PATCH 07/12] Stop a stale agent SMART row lending a replaced disk its identity The linked host agent's SMART merge falls back to matching a Proxmox disk by device path, and a path names a slot, not a disk. When a disk is swapped before the agent reports again, or after the agent went silent and kept its last rows, the row for the old disk matched the replacement. It filled or, for SAS, promoted the old serial onto it, which keyed the replacement to the old disk's canonical resource and metric history, and lent it the old disk's temperature and failed health. For SAS the registry's same-path join did the same at the canonical level even when monitoring kept the rows apart. Both now refuse a row whose identity contradicts the disk's, judged only on what both producers report like for like. WWNs count for every disk. Serials count only where Proxmox's serial is the drive's own, as smartctl's is: NVMe namespaces, and disks presented with SCSI vendor "ATA", whose serial udev's ata_id reads from IDENTIFY. Elsewhere Proxmox's serial may be a SAS address, a SCSI designator or a USB bridge's own serial (#1595). Reporter spellings are not disagreement: udev writes whitespace in serials as underscores, its T10 designator for an ATA drive puts "ATA" and the model before the drive serial, and its ID_WWN keeps only the first 64 bits of an NAA 6 identifier. The WWN match step compares normalized identity too, so the agent's 5-c50-a1b2c3d4 spelling finds the Proxmox 0x5000c500a1b2c3d4 disk. Proxmox's disk inventory reports a serial or WWN udev cannot read as the literal "unknown". The poller recorded it as a collected serial, so the collection state hid the gap and the agent's serial never filled it. It is now recorded as empty, and retained evidence carries only a usable previous serial so an older record cannot restore it. Identity keys already ignored the placeholder. --- .../v6/internal/subsystems/agent-lifecycle.md | 11 + .../v6/internal/subsystems/monitoring.md | 52 +++- .../internal/subsystems/unified-resources.md | 16 ++ .../issue1595_collection_trust_test.go | 245 ++++++++++++++++++ internal/monitoring/merge_temps_test.go | 6 +- internal/monitoring/monitor.go | 57 +++- .../monitoring/monitor_host_agents_test.go | 136 ++++++++++ internal/monitoring/monitor_pve.go | 27 +- .../physical_disk_roundtrip_test.go | 6 +- internal/unifiedresources/registry.go | 10 +- pkg/diskinventory/identity_conflict.go | 61 +++++ pkg/diskinventory/identity_conflict_test.go | 36 +++ 12 files changed, 643 insertions(+), 20 deletions(-) create mode 100644 pkg/diskinventory/identity_conflict.go create mode 100644 pkg/diskinventory/identity_conflict_test.go diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index dd1d022b02..42680ec7ab 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -237,6 +237,17 @@ state; the monitoring readback requires an actual Proxmox source observation. This leaves Agent source identity, report admission and command authority unchanged. The repeated-cycle regression is `TestPhysicalDiskSkippedPollDoesNotPromoteAgentOnlySMARTToPVEInventory`. +A linked Agent's SMART row that matches a PVE disk only by device path is +refused when its WWN contradicts the disk's, or its serial does where both +producers read the drive's own serial (NVMe, and disks presented with SCSI +vendor `ATA`), +and the registry's SAS path join refuses a contradicting WWN, so a silent +Agent's retained row for a swapped-out disk lends the replacement neither its +serial, and with it the old disk's canonical resource, nor its readings. +Without comparable identity (a SAS, SCSI or USB replacement with no WWN on one +side) the row still matches. Report admission and the SMART wire contract are +unchanged; the regression is +`TestHostAgentSMARTRowForSwappedOutDiskDoesNotLendItsIdentity`. Assistant historical metric wiring uses the current monitor's retained store and registry metrics coordinates. Historical reads do not alter enrollment, diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 6d91671d13..f1342aaf73 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -1508,6 +1508,51 @@ later coarse healthy value and lets explicit SMART endurance replace contradictory Proxmox wearout. Missing permission, ambiguous identity, standby, and absent SMART fields remain neutral rather than borrowing telemetry from another disk. +The device-path match is the last resort, and a path names a slot, not a +disk: the row there may describe the slot's previous occupant, because the +agent has not reported since a swap or went silent and keeps its last rows. +`mergeHostAgentSMARTIntoDisks` refuses such a row when its identity +contradicts the Proxmox disk's (`diskinventory.HardwareIdentityConflict`): +nothing agrees across serial and WWN, and a field both producers report like +for like disagrees. WWNs are compared for every disk. Serials are compared +only where Proxmox's serial is the drive's own, as smartctl's is: a Proxmox +disk typed `nvme` against an NVMe row, and one presented with SCSI vendor +`ATA` (libata, or a SAS HBA's SCSI-ATA translation, where udev's `ata_id` +reads IDENTIFY) and not typed `usb` or `sas` against an ATA row. Any other +Proxmox serial may be a SAS address or SCSI designator (#1595) or a USB +bridge's own serial, so it counts toward agreement only; a USB bridge that +does expose the drive's serial is excluded too unless it reports vendor `ATA`. +Reporter spellings are not disagreement: serials compare on letters and digits +only (udev writes whitespace as underscores), a T10 vendor designator for an +ATA drive (`ATA`, the model, then the serial) is no evidence against the serial +it ends with, and a WWN that begins with the other is no evidence (udev's `ID_WWN` keeps only +the first 64 bits of a 128-bit NAA 6 identifier). The refusal comes after the +unique-path check, so a second row on the path still makes it ambiguous. The +WWN match itself compares through `HardwareIdentityMatch`, so the agent's +`5-c50-a1b2c3d4` spelling finds the Proxmox `0x5000c500a1b2c3d4` disk. The +registry's SAS path join applies the same WWN refusal (unified-resources). +Proxmox's disk inventory (`PVE::Diskmanage`) writes the literal `unknown` for +a serial or WWN udev cannot read; the poller records it as empty, so the +serial's collection state reads `missing` and the agent's serial or WWN can +fill it, where the placeholder used to read as a collected serial. Retained +evidence carries a previous serial only when it is a usable identity, so a +record from before this change cannot restore the placeholder. Identity keys +already ignored the placeholder, so the empty value keys the same; a disk whose +serial the linked agent now fills keys its Proxmox SMART samples on that serial +instead of its WWN or path, as the agent's own samples already were. Before the +guard, the retained row of a swapped-out disk filled or promoted the old serial +onto the replacement, which keyed it to the old disk's canonical resource and +history, and lent it the old disk's temperature and failed health +(`TestHostAgentSMARTRowForSwappedOutDiskDoesNotLendItsIdentity`, +`TestHostAgentSMARTMergeRefusesPathMatchAcrossContradictingIdentity`, +`TestRegistrySASPathJoinRefusesContradictingWWN`, +`TestProxmoxUnknownDiskIdentityIsRecordedAsUnreported`, +`TestHardwareIdentityConflict`). Without comparable evidence there is nothing +to act on: a row without a usable serial or WWN still matches its path, and a +replacement whose serial is not comparable (SAS, SCSI, USB) still takes a +stale row's identity when either record lacks a WWN. The registry's seeded +source mapping, which a restart reuses by the slot-shaped Proxmox source ID, +does not apply this guard yet. Retaining evidence a disk poll could not collect follows the same identity rule. `previousPhysicalDiskEvidence` looks up the earlier record by serial, then by WWN, each compared through `diskinventory.HardwareIdentityMatch` against @@ -1529,10 +1574,9 @@ serial, which keyed it to the old disk's canonical resource and SMART metric series, so every later poll carried the borrowed serial forward (`TestPhysicalDiskReplacementInSameSlotDoesNotInheritPreviousSerial`, `TestPreviousPhysicalDiskEvidenceRejectsSlotMatchAcrossConflictingIdentity`). -This lookup is the only place the rule applies: a replacement that reports no -usable serial or WWN cannot be told apart from the same disk and still -inherits, and the linked agent's SMART merge, which runs first, can still fill -or promote a serial from a stale SMART row it matched by device path. +A replacement that reports no usable serial or WWN cannot be told apart from +the same disk and still inherits. The linked agent's SMART merge, which runs +first, refuses a contradicting device-path row on its own terms (above). Negative percentage-used counters remain unknown; values above 100 clamp to exhausted before deriving remaining life, so invalid or over-limit controller data cannot wrap into a fabricated healthy value. diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index bc3d781246..9d03848610 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -6431,6 +6431,22 @@ ambiguity test rejects multiple opposite-source candidates in both directions. These are synthetic source proofs, not appliance acceptance or a claim about which collector produced a reporter's row. +### Linked SAS path join refuses a contradicting WWN + +`resolveLinkedPhysicalDisk` falls back to joining a Proxmox disk to its linked +agent's disk on the same path when the agent reports SAS, because Proxmox may +report a SAS address as the serial (#1595). That path fallback is refused when +both sides carry WWNs that name different disks +(`diskinventory.HardwareIdentityConflict`, under which the agent's NAA field +spelling equals the Proxmox WWN and udev's 64-bit NAA 6 prefix is no +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. + ### Drawer tab selection survives a transient snapshot change (#1723) The shared resource detail drawer keeps the operator's selected tab while a diff --git a/internal/monitoring/issue1595_collection_trust_test.go b/internal/monitoring/issue1595_collection_trust_test.go index daa3cf2605..7a52f71d8b 100644 --- a/internal/monitoring/issue1595_collection_trust_test.go +++ b/internal/monitoring/issue1595_collection_trust_test.go @@ -1,16 +1,21 @@ package monitoring import ( + "context" "encoding/json" "os" "path/filepath" + "strings" "testing" "time" + "github.com/rcourtman/pulse-go-rewrite/internal/alerts" + "github.com/rcourtman/pulse-go-rewrite/internal/config" "github.com/rcourtman/pulse-go-rewrite/internal/models" "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" "github.com/rcourtman/pulse-go-rewrite/pkg/diskinventory" "github.com/rcourtman/pulse-go-rewrite/pkg/metrics" + "github.com/rcourtman/pulse-go-rewrite/pkg/proxmox" ) type issue1595TopologyFixture struct { @@ -367,3 +372,243 @@ func TestWriteHostSMARTMetricsRecordsOnlyCollectedTemperatures(t *testing.T) { } } } + +// A SMART row found only by device path may describe the slot's previous +// occupant. It is refused when its identity contradicts the Proxmox disk's, +// judged only on what both producers report like for like: WWNs for every +// disk, serials only where Proxmox's serial is the drive's own, for NVMe and +// for disks the kernel reaches through libata (vendor "ATA"). A SAS disk's +// Proxmox serial may be a transport address and a USB bridge's is its own. +// Reporter spellings of one identity (udev's underscores, a T10 designator +// ending with the drive serial, the agent's NAA fields, udev's 64-bit NAA 6 +// prefix) are not disagreement. +func TestHostAgentSMARTMergeRefusesPathMatchAcrossContradictingIdentity(t *testing.T) { + type mergeCase struct { + name string + disk models.PhysicalDisk + smart models.HostDiskSMART + // others are further rows on the same path, which make it ambiguous. + others []models.HostDiskSMART + merged bool + serial string + } + smartctlSerial := &diskinventory.CollectionStatus{Serial: diskinventory.Available("smartctl")} + pveDisk := func(device, vendor, serial, wwn, diskType string) models.PhysicalDisk { + return models.PhysicalDisk{ + ID: "pve1-node1-" + device, Node: "node1", Instance: "pve1", DevPath: "/dev/" + device, + Vendor: vendor, Serial: serial, WWN: wwn, Type: diskType, Health: "PASSED", + } + } + smartRow := func(device, serial, wwn, diskType string) models.HostDiskSMART { + return models.HostDiskSMART{ + Device: "/dev/" + device, Serial: serial, WWN: wwn, Type: diskType, + Health: "FAILED", Temperature: 47, Collection: diskinventory.CloneStatus(smartctlSerial), + } + } + for _, tc := range []mergeCase{ + { + name: "SAS replacement beside a stale row with another WWN", + disk: pveDisk("sda", "SEAGATE", "5000c500b0000001", "0x5000c500b0000001", "unknown"), + smart: smartRow("sda", "ZR5OLD0001", "naa.5000c500a0000001", "sas"), + }, + { + name: "SATA replacement beside a stale row with another serial", + disk: pveDisk("sdb", "ATA", "WD-NEW0001", "", "hdd"), + smart: smartRow("sdb", "WD-OLD0001", "", "sata"), + }, + { + name: "same SAS disk whose agent row has no WWN", + disk: pveDisk("sdc", "SEAGATE", "5000c500a0000003", "0x5000c500a0000003", "unknown"), + smart: smartRow("sdc", "ZR5TESTA0003", "", "sas"), + merged: true, serial: "ZR5TESTA0003", + }, + { + name: "same SAS disk renamed, agent WWN in smartctl NAA fields", + disk: pveDisk("sdd", "SEAGATE", "5000c500a0000004", "0x5000c500a0000004", "unknown"), + smart: smartRow("sdx", "ZR5TESTA0004", "5-c50-a0000004", "sas"), + merged: true, serial: "ZR5TESTA0004", + }, + { + name: "same disk with udev's spelling of the serial", + disk: pveDisk("sde", "ATA", "WD-WX_1234", "", "hdd"), + smart: smartRow("sde", "WD-WX 1234", "", "sata"), + merged: true, serial: "WD-WX_1234", + }, + { + name: "NVMe replacement whose serial looks like a SAS address", + disk: pveDisk("nvme0n1", "", "50026B7282A0FB69", "", "nvme"), + smart: smartRow("nvme0n1", "50026B7282A0FB11", "", "nvme"), + }, + { + name: "ambiguous path where only one row contradicts", + disk: pveDisk("sdg", "ATA", "WD-NEW0007", "", "hdd"), + smart: smartRow("sdg", "WD-OLD0007", "", "sata"), + others: []models.HostDiskSMART{ + smartRow("sdg", "", "", "sata"), + }, + }, + { + name: "same disk in a USB enclosure whose serial udev reports", + disk: pveDisk("sdh", "JMicron", "000000000000000F1E2D", "", "usb"), + smart: smartRow("sdh", "WD-WX0008", "", "sata"), + merged: true, serial: "000000000000000F1E2D", + }, + { + name: "same SSD in a USB enclosure Proxmox types ssd", + disk: pveDisk("sdj", "ASMT", "00000000000000000009", "", "ssd"), + smart: smartRow("sdj", "S3Z9NB0K123456X", "", "sata"), + merged: true, serial: "00000000000000000009", + }, + { + name: "same disk behind a T10 designator ending with its serial", + disk: pveDisk("sdi", "ATA", "ATA_ST4000NM000A-2HZ_ZC1ABCDE", "", "unknown"), + smart: smartRow("sdi", "ZC1ABCDE", "", "sata"), + merged: true, serial: "ATA_ST4000NM000A-2HZ_ZC1ABCDE", + }, + { + name: "same NAA 6 volume with udev's 64-bit WWN", + disk: pveDisk("sdf", "HP", "PDNLH0BRH8V0AB", "0x600508b1001c4d5e", "hdd"), + smart: smartRow("sdf", "", "naa.600508b1001c4d5e7f8a9b0c1d2e3f40", "scsi"), + merged: true, serial: "PDNLH0BRH8V0AB", + }, + } { + t.Run(tc.name, func(t *testing.T) { + got := mergeHostAgentSMARTIntoDisks( + []models.PhysicalDisk{tc.disk}, + []models.Node{{Name: "node1", LinkedAgentID: "agent-1"}}, + []models.Host{{ID: "agent-1", Sensors: models.HostSensorSummary{ + SMART: append([]models.HostDiskSMART{tc.smart}, tc.others...), + }}}, + )[0] + if !tc.merged { + if got.Serial != tc.disk.Serial || got.WWN != tc.disk.WWN || got.Type != tc.disk.Type || + got.Temperature != 0 || got.Health != "PASSED" { + t.Fatalf("contradicting row was merged into the disk: %+v", got) + } + return + } + if got.Serial != tc.serial || got.Temperature != 47 || got.Health != "FAILED" { + t.Fatalf("same disk's row was not merged: serial %q (want %q), temperature %d, health %q", + got.Serial, tc.serial, got.Temperature, got.Health) + } + }) + } +} + +// Proxmox's disk inventory reports a serial or WWN udev cannot read as the +// literal "unknown". The poller records it as unreported rather than as a +// collected serial; the disk's identity keys were already blind to it. +func TestProxmoxUnknownDiskIdentityIsRecordedAsUnreported(t *testing.T) { + t.Setenv("PULSE_DATA_DIR", t.TempDir()) + state := models.NewState() + state.UpdateNodesForInstance("pve1", []models.Node{{ + ID: "pve1-node1", Name: "node1", Instance: "pve1", Status: "online", LastSeen: time.Now(), + }}) + alertManager := alerts.NewManager() + t.Cleanup(alertManager.Stop) + m := &Monitor{ + state: state, alertManager: alertManager, + startTime: time.Now().Add(-time.Hour), lastPhysicalDiskPoll: make(map[string]time.Time), + } + // A record from before the placeholder was recorded as unreported must + // not carry it back in as the disk's last known serial. + state.UpdatePhysicalDisks("pve1", []models.PhysicalDisk{{ + ID: unifiedresources.ProxmoxPhysicalDiskSourceID("pve1", "node1", "/dev/sdb", "", ""), + Node: "node1", Instance: "pve1", DevPath: "/dev/sdb", Serial: "unknown", WWN: "unknown", + Collection: &diskinventory.CollectionStatus{Serial: diskinventory.Available("proxmox_disks")}, + }}) + client := &slotDiskPVEClient{} + client.setDisk(proxmox.Disk{ + DevPath: "/dev/sdb", Model: "USB3.0 Bridge", Serial: "unknown", WWN: "unknown", + Type: "hdd", Health: "PASSED", Wearout: 100, Size: 1000204886016, + }) + started := time.Now() + m.maybePollPhysicalDisksAsync(context.Background(), "pve1", &config.PVEInstance{}, client, + []proxmox.Node{{Node: "node1", Status: "online"}}, map[string]string{"node1": "online"}, nil) + deadline := started.Add(3 * time.Second) + for disks := state.GetSnapshot().PhysicalDisks; len(disks) != 1 || disks[0].LastChecked.Before(started); disks = state.GetSnapshot().PhysicalDisks { + if time.Now().After(deadline) { + t.Fatal("physical disk poll did not land in state") + } + time.Sleep(10 * time.Millisecond) + } + got := state.GetSnapshot().PhysicalDisks[0] + if got.Serial != "" || got.WWN != "" { + t.Fatalf("placeholder identity recorded as reported: serial %q, wwn %q", got.Serial, got.WWN) + } + if got.Collection == nil || got.Collection.Serial.State != diskinventory.FieldMissing { + t.Fatalf("serial collection = %+v, want missing", got.Collection) + } + placeholder := got + placeholder.Serial, placeholder.WWN = "unknown", "unknown" + if unifiedresources.PhysicalDiskMetricID(got) != unifiedresources.PhysicalDiskMetricID(placeholder) { + t.Fatalf("metric key moved: %q, was %q", + unifiedresources.PhysicalDiskMetricID(got), unifiedresources.PhysicalDiskMetricID(placeholder)) + } +} + +// The registry joins a Proxmox disk to its linked agent's disk on the same +// path when the agent reports SAS, because Proxmox puts the SAS address in +// the serial (#1595). A WWN naming another disk refuses that join: the agent's +// row may be the slot's previous occupant, retained by a silent agent, and the +// join used to hand the replacement that disk's serial, readings and canonical +// resource. +func TestRegistrySASPathJoinRefusesContradictingWWN(t *testing.T) { + const replacementWWN = "0x5000c500bbbb0002" + for _, tc := range []struct { + name string + agentWWN string + joined bool + }{ + {name: "retained row of the previous occupant", agentWWN: "5-c50-aaaa0001"}, + {name: "same disk in the agent's WWN spelling", agentWWN: "5-c50-bbbb0002", joined: true}, + {name: "same disk without an agent WWN", agentWWN: "", joined: true}, + } { + t.Run(tc.name, func(t *testing.T) { + 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: "ZR5AGENT0001", WWN: tc.agentWWN, Type: "sas", + Health: "FAILED", Temperature: 52, + Collection: &diskinventory.CollectionStatus{ + Serial: diskinventory.Available("smartctl"), + Temperature: diskinventory.Available("smartctl"), + }, + }}}, + } + replacement := models.PhysicalDisk{ + ID: "pve1-node1--dev-sdb", Node: "node1", Instance: "pve1", DevPath: "/dev/sdb", + Serial: strings.TrimPrefix(replacementWWN, "0x"), WWN: replacementWWN, Type: "unknown", + Health: "PASSED", Wearout: -1, LastChecked: now, + } + registry := unifiedresources.NewRegistry(nil) + registry.IngestSnapshot(models.StateSnapshot{ + Nodes: []models.Node{node}, Hosts: []models.Host{host}, PhysicalDisks: []models.PhysicalDisk{replacement}, + }) + + var proxmoxDisk *unifiedresources.Resource + disks := registry.ListByType(unifiedresources.ResourceTypePhysicalDisk) + for index := range disks { + if hasIssue1595Source(disks[index].Sources, unifiedresources.SourceProxmox) { + proxmoxDisk = &disks[index] + } + } + if proxmoxDisk == nil || proxmoxDisk.PhysicalDisk == nil { + t.Fatalf("no canonical Proxmox disk among %d disks", len(disks)) + } + joined := hasIssue1595Source(proxmoxDisk.Sources, unifiedresources.SourceAgent) + if joined != tc.joined || (tc.joined && len(disks) != 1) || (!tc.joined && len(disks) != 2) { + t.Fatalf("joined = %v with %d disks, want joined = %v", joined, len(disks), tc.joined) + } + if !tc.joined && (proxmoxDisk.PhysicalDisk.Serial == "ZR5AGENT0001" || + proxmoxDisk.PhysicalDisk.Temperature != 0 || proxmoxDisk.PhysicalDisk.Health == "FAILED") { + t.Fatalf("replacement took the retained row's identity or readings: %+v", proxmoxDisk.PhysicalDisk) + } + }) + } +} diff --git a/internal/monitoring/merge_temps_test.go b/internal/monitoring/merge_temps_test.go index 18f884afa7..a95ada4b46 100644 --- a/internal/monitoring/merge_temps_test.go +++ b/internal/monitoring/merge_temps_test.go @@ -435,7 +435,7 @@ func TestMergeHostAgentSMARTIntoDisks_PreservesExistingProxmoxIdentity(t *testin DevPath: "/dev/sdb", Model: "Proxmox Model", Serial: "PVE-SERIAL", - WWN: "pve-wwn", + WWN: "0x5000c500a1b2c3d4", Type: "sas", Size: 100, Health: "PASSED", @@ -453,7 +453,7 @@ func TestMergeHostAgentSMARTIntoDisks_PreservesExistingProxmoxIdentity(t *testin Device: "/dev/sdb", Model: "Agent Model", Serial: "AGENT-SERIAL", - WWN: "agent-wwn", + WWN: "5-c50-a1b2c3d4", // the same disk, in smartctl's spelling Type: "sata", SizeBytes: 200, Temperature: 35, @@ -466,7 +466,7 @@ func TestMergeHostAgentSMARTIntoDisks_PreservesExistingProxmoxIdentity(t *testin } got := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts)[0] - if got.Model != "Proxmox Model" || got.Serial != "PVE-SERIAL" || got.WWN != "pve-wwn" || got.Type != "sas" { + if got.Model != "Proxmox Model" || got.Serial != "PVE-SERIAL" || got.WWN != "0x5000c500a1b2c3d4" || got.Type != "sas" { t.Fatalf("existing Proxmox identity was overwritten: %+v", got) } if got.Size != 100 || got.Temperature != 41 || got.StorageGroup != "api-pool" { diff --git a/internal/monitoring/monitor.go b/internal/monitoring/monitor.go index f91fb9fc3f..a402731aa4 100644 --- a/internal/monitoring/monitor.go +++ b/internal/monitoring/monitor.go @@ -560,11 +560,12 @@ func mergeHostAgentSMARTIntoDisks(disks []models.PhysicalDisk, nodes []models.No return &smartData[matchIndex] } - // Try to match by WWN (most reliable) + // Try to match by WWN (most reliable). PVE and the agent spell one WWN + // differently (0x5000c500a1b2c3d4 against smartctl's 5-c50-a1b2c3d4), + // so compare the normalized identity, not the reported string. if diskinventory.IsUsableHardwareID(updated[i].WWN) { matched = uniqueMatch(func(candidate models.HostDiskSMART) bool { - return diskinventory.IsUsableHardwareID(candidate.WWN) && - strings.EqualFold(candidate.WWN, updated[i].WWN) && + return diskinventory.HardwareIdentityMatch("", updated[i].WWN, "", candidate.WWN) && diskTopologyCompatible(updated[i].Controller, updated[i].Target, candidate.Controller, candidate.Target) }) } @@ -578,7 +579,12 @@ func mergeHostAgentSMARTIntoDisks(disks []models.PhysicalDisk, nodes []models.No }) } - // Last resort: match by device path + // Last resort: match by device path. A path names a slot, not a disk, + // so the row there may describe the slot's previous occupant: the agent + // has not reported since the swap, or it went silent and its last rows + // are retained. A row whose identity contradicts the disk's is refused, + // or it would fill or promote the old serial onto the replacement, + // which would then take the old disk's canonical resource and history. if matched == nil { normalizedDevPath := normalizeSMARTDeviceIdentifier(updated[i].DevPath) matched = uniqueMatch(func(candidate models.HostDiskSMART) bool { @@ -587,6 +593,15 @@ func mergeHostAgentSMARTIntoDisks(disks []models.PhysicalDisk, nodes []models.No normalizedDiskDev == normalizedDevPath && diskTopologyCompatible(updated[i].Controller, updated[i].Target, candidate.Controller, candidate.Target) }) + if matched != nil && hostAgentSMARTIdentityConflicts(updated[i], *matched) { + log.Debug(). + Str("node", updated[i].Node). + Str("device", updated[i].DevPath). + Str("serial", updated[i].Serial). + Str("smartSerial", matched.Serial). + Msg("Skipping host agent SMART row whose disk identity contradicts the Proxmox disk at the same path") + matched = nil + } } if matched == nil { @@ -703,6 +718,40 @@ func diskTopologyCompatible(leftController, leftTarget, rightController, rightTa return true } +// hostAgentSMARTIdentityConflicts reports whether a SMART row found only by +// device path names a different disk than the Proxmox disk at that path. +// WWNs are compared for every disk; serials only where both producers read +// the drive's own serial (hostAgentSMARTSerialComparable). Any other Proxmox +// serial still counts toward agreement. +func hostAgentSMARTIdentityConflicts(disk models.PhysicalDisk, smart models.HostDiskSMART) bool { + if diskinventory.HardwareIdentityMatch(disk.Serial, disk.WWN, smart.Serial, smart.WWN) { + return false + } + serial := "" + if hostAgentSMARTSerialComparable(disk, smart) { + serial = disk.Serial + } + return diskinventory.HardwareIdentityConflict(serial, disk.WWN, smart.Serial, smart.WWN) +} + +// hostAgentSMARTSerialComparable reports whether the Proxmox serial was read +// from the drive itself, as smartctl's is. Proxmox takes the serial from udev: +// for an NVMe namespace that is the controller's serial, and for a disk +// presented with SCSI vendor "ATA" (libata, or a SAS HBA's SCSI-ATA +// translation) udev's ata_id reads it from IDENTIFY. Any other disk's Proxmox +// serial may be a SAS address or SCSI designator from VPD page 0x83 (#1595), +// or a USB bridge's own serial. +func hostAgentSMARTSerialComparable(disk models.PhysicalDisk, smart models.HostDiskSMART) bool { + agentType := strings.ToLower(strings.TrimSpace(smart.Type)) + switch strings.ToLower(strings.TrimSpace(disk.Type)) { + case "nvme": + return agentType == "nvme" + case "usb", "sas": + return false + } + return agentType == "sata" && strings.EqualFold(strings.TrimSpace(disk.Vendor), "ATA") +} + func shouldPromoteHostAgentSerial(disk models.PhysicalDisk, smart models.HostDiskSMART) bool { if smart.Collection != nil && strings.EqualFold(strings.TrimSpace(smart.Type), "sas") && diff --git a/internal/monitoring/monitor_host_agents_test.go b/internal/monitoring/monitor_host_agents_test.go index 8fc230e0b6..a55f0881a3 100644 --- a/internal/monitoring/monitor_host_agents_test.go +++ b/internal/monitoring/monitor_host_agents_test.go @@ -1,6 +1,7 @@ package monitoring import ( + "context" "encoding/json" "errors" "fmt" @@ -23,6 +24,7 @@ import ( "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" agentsdocker "github.com/rcourtman/pulse-go-rewrite/pkg/agents/docker" agentshost "github.com/rcourtman/pulse-go-rewrite/pkg/agents/host" + "github.com/rcourtman/pulse-go-rewrite/pkg/diskinventory" "github.com/rcourtman/pulse-go-rewrite/pkg/metrics" "github.com/rcourtman/pulse-go-rewrite/pkg/proxmox" ) @@ -6945,3 +6947,137 @@ func TestSilentLinkedAgentStopsFeedingNodeDisk(t *testing.T) { t.Fatalf("agent reporting again: node disk = %+v from %q, agent %q; want the agent's summary and online", disk, source, host.Status) } } + +// A disk swapped into a slot must not take its identity from the linked host +// agent's row for the slot's previous occupant. A silent agent keeps its last +// rows, and that row used to match the replacement by device path alone: it +// filled a serial Proxmox reported as "unknown" (recorded as unreported, so +// the agent can fill it) or, for SAS, promoted its serial over the SAS address +// Proxmox reports, and the registry joined a SAS replacement to the agent's +// disk on the same path. Either way the replacement took the old disk's +// serial, canonical resource and history, temperature and failed health. +func TestHostAgentSMARTRowForSwappedOutDiskDoesNotLendItsIdentity(t *testing.T) { + const ( + oldSerial = "ZA1OLD0001" + oldAgentWWN = "5-c50-aaaa0001" // smartctl's NAA field spelling + oldPVEWWN = "0x5000c500aaaa0001" + newPVEWWN = "0x5000c500bbbb0002" + ) + for _, transport := range []struct { + agentType string + // pveSerial is what Proxmox reports for a disk with the given WWN. + pveSerial func(wwn string) string + // replacementSerial is the serial the replacement's record keeps. + replacementSerial string + serialState diskinventory.FieldState + }{ + { + agentType: "sata", + pveSerial: func(string) string { return "unknown" }, + replacementSerial: "", + serialState: diskinventory.FieldMissing, + }, + { + agentType: "sas", + pveSerial: func(wwn string) string { return strings.TrimPrefix(wwn, "0x") }, + replacementSerial: strings.TrimPrefix(newPVEWWN, "0x"), + serialState: diskinventory.FieldAvailable, + }, + } { + t.Run(transport.agentType, func(t *testing.T) { + t.Setenv("PULSE_DATA_DIR", t.TempDir()) + now := time.Now() + state := models.NewState() + state.UpdateNodesForInstance("pve1", []models.Node{{ + ID: "pve1-node1", Name: "node1", Instance: "pve1", Status: "online", + LastSeen: now, LinkedAgentID: "agent-1", + }}) + state.UpsertHost(models.Host{ + ID: "agent-1", Hostname: "node1", LinkedNodeID: "pve1-node1", + Status: "online", IntervalSeconds: 30, LastSeen: now, + Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{{ + Device: "/dev/sdb", Serial: oldSerial, WWN: oldAgentWWN, Type: transport.agentType, + Health: "FAILED", Temperature: 52, + Collection: &diskinventory.CollectionStatus{ + Serial: diskinventory.Available("smartctl"), + Temperature: diskinventory.Available("smartctl"), + }, + }}}, + }) + alertManager := alerts.NewManager() + t.Cleanup(alertManager.Stop) + adapter := unifiedresources.NewMonitorAdapter(unifiedresources.NewRegistry(nil)) + m := &Monitor{ + state: state, resourceStore: adapter, alertManager: alertManager, + metricsHistory: NewMetricsHistory(100, time.Hour), + startTime: now.Add(-time.Hour), lastPhysicalDiskPoll: make(map[string]time.Time), + } + client := &slotDiskPVEClient{} + pveDisk := func(wwn string) proxmox.Disk { + return proxmox.Disk{ + DevPath: "/dev/sdb", Model: "ST4000NM000A", Serial: transport.pveSerial(wwn), WWN: wwn, + Type: "hdd", Health: "PASSED", Wearout: 100, Size: 4000787030016, + } + } + poll := func() (models.PhysicalDisk, *unifiedresources.PhysicalDiskView) { + t.Helper() + adapter.PopulateFromSnapshot(state.GetSnapshot()) + started := time.Now() + delete(m.lastPhysicalDiskPoll, "pve1") + m.maybePollPhysicalDisksAsync(context.Background(), "pve1", &config.PVEInstance{}, client, + []proxmox.Node{{Node: "node1", Status: "online"}}, map[string]string{"node1": "online"}, nil) + deadline := time.Now().Add(3 * time.Second) + for { + if disks := state.GetSnapshot().PhysicalDisks; len(disks) == 1 && !disks[0].LastChecked.Before(started) { + adapter.PopulateFromSnapshot(state.GetSnapshot()) + var proxmoxViews []*unifiedresources.PhysicalDiskView + for _, view := range adapter.PhysicalDisks() { + if _, ok := view.SourceStatus(unifiedresources.SourceProxmox); ok { + proxmoxViews = append(proxmoxViews, view) + } + } + if len(proxmoxViews) != 1 { + t.Fatalf("canonical Proxmox physical disks = %d, want one", len(proxmoxViews)) + } + return disks[0], proxmoxViews[0] + } + if time.Now().After(deadline) { + t.Fatal("physical disk poll did not land in state") + } + time.Sleep(10 * time.Millisecond) + } + } + + client.setDisk(pveDisk(oldPVEWWN)) + original, originalView := poll() + if original.Serial != oldSerial || originalView.Serial() != oldSerial || + original.Collection == nil || original.Collection.Serial.State != diskinventory.FieldAvailable { + t.Fatalf("agent serial not merged into the disk it describes: state %q, canonical %q, collection %+v", + original.Serial, originalView.Serial(), original.Collection) + } + + // The agent stops reporting, keeping its rows, and the disk is replaced. + state.TouchHost("agent-1", now.Add(-hostAgentHealthWindow(30)-time.Minute)) + m.evaluateHostAgents(now) + client.setDisk(pveDisk(newPVEWWN)) + for round := 1; round <= 2; round++ { + got, view := poll() + if got.Serial != transport.replacementSerial || view.Serial() == oldSerial { + t.Fatalf("poll %d: replacement took the retained row's serial: state %q, canonical %q", + round, got.Serial, view.Serial()) + } + if view.ID() == originalView.ID() { + t.Fatalf("poll %d: replacement took over the old disk's canonical resource %q", round, view.ID()) + } + if got.Temperature != 0 || got.Health != "PASSED" || view.Temperature() != 0 || view.Health() != "PASSED" { + t.Fatalf("poll %d: replacement took the old disk's readings: state %d/%q, canonical %d/%q", + round, got.Temperature, got.Health, view.Temperature(), view.Health()) + } + if got.WWN != newPVEWWN || + got.Collection == nil || got.Collection.Serial.State != transport.serialState { + t.Fatalf("poll %d: replacement wwn %q, serial collection %+v", round, got.WWN, got.Collection) + } + } + }) + } +} diff --git a/internal/monitoring/monitor_pve.go b/internal/monitoring/monitor_pve.go index 4456d56f65..1e11c0b3d5 100644 --- a/internal/monitoring/monitor_pve.go +++ b/internal/monitoring/monitor_pve.go @@ -1105,8 +1105,8 @@ func (m *Monitor) maybePollPhysicalDisksAsync( DevPath: disk.DevPath, Model: disk.Model, Vendor: disk.Vendor, - Serial: disk.Serial, - WWN: disk.WWN, + Serial: proxmoxReportedDiskIdentity(disk.Serial), + WWN: proxmoxReportedDiskIdentity(disk.WWN), Type: disk.Type, Size: disk.Size, Health: normalizeProxmoxDiskHealth(disk.Health), @@ -1121,7 +1121,7 @@ func (m *Monitor) maybePollPhysicalDisksAsync( }, LastChecked: time.Now(), } - if strings.TrimSpace(disk.Serial) != "" { + if strings.TrimSpace(physicalDisk.Serial) != "" { physicalDisk.Collection.Serial = diskinventory.Available("proxmox_disks") } else { physicalDisk.Collection.Serial = diskinventory.Missing("proxmox_disks", "disk serial was not reported") @@ -1220,6 +1220,22 @@ func normalizeProxmoxDiskHealth(health string) string { } } +// proxmoxUnreportedDiskIdentity is what PVE's disk inventory +// (PVE::Diskmanage) writes into a disk's serial and wwn when udev has none. +const proxmoxUnreportedDiskIdentity = "unknown" + +// proxmoxReportedDiskIdentity records PVE's placeholder for an unreported +// serial or WWN as empty. Kept verbatim, the placeholder read as a collected +// serial, so the disk's collection state hid the gap and the linked host +// agent's serial or WWN never filled it. Identity keys are unaffected: +// diskinventory.IsUsableHardwareID already rejected the placeholder. +func proxmoxReportedDiskIdentity(value string) string { + if strings.EqualFold(strings.TrimSpace(value), proxmoxUnreportedDiskIdentity) { + return "" + } + return value +} + // physicalDisksFromHostAgentSMART builds PhysicalDisk entries for a node from // its linked host agent's SMART inventory. This is the fallback when the // Proxmox disks/list query fails: PVE probes SMART per disk inside that call, @@ -1371,8 +1387,11 @@ func preserveUnavailablePhysicalDiskEvidence(current, previous models.PhysicalDi if current.Collection == nil { return current } + // Only an identity is worth keeping: a record from before Proxmox's + // "unknown" was recorded as unreported would otherwise restore it. if current.Collection.Serial.State != diskinventory.FieldAvailable && - strings.TrimSpace(current.Serial) == "" { + strings.TrimSpace(current.Serial) == "" && + diskinventory.IsUsableHardwareID(previous.Serial) { current.Serial = previous.Serial } if current.Collection.Temperature.State != diskinventory.FieldAvailable && diff --git a/internal/monitoring/physical_disk_roundtrip_test.go b/internal/monitoring/physical_disk_roundtrip_test.go index 6023113a3d..2c4bb0833b 100644 --- a/internal/monitoring/physical_disk_roundtrip_test.go +++ b/internal/monitoring/physical_disk_roundtrip_test.go @@ -809,9 +809,9 @@ func (client *slotDiskPVEClient) GetDisks(context.Context, string) ([]proxmox.Di // it: the registry keys the disk on that serial, so the replacement would take // over the old disk's canonical resource, and every later poll would copy the // borrowed serial forward again. Current Proxmox spells a missing serial as -// "unknown", which the poller records as reported; an empty serial comes from -// producers that omit the field, such as the host-agent fallback rows. The -// fake client below stands in for any of them. +// "unknown", which the poller records as empty, as it does for producers that +// omit the field, such as the host-agent fallback rows. The fake client below +// stands in for any of them. func TestPhysicalDiskReplacementInSameSlotDoesNotInheritPreviousSerial(t *testing.T) { t.Setenv("PULSE_DATA_DIR", t.TempDir()) const ( diff --git a/internal/unifiedresources/registry.go b/internal/unifiedresources/registry.go index 6c9a786cad..7d59ac78b4 100644 --- a/internal/unifiedresources/registry.go +++ b/internal/unifiedresources/registry.go @@ -3272,8 +3272,14 @@ func (rr *ResourceRegistry) resolveLinkedPhysicalDisk(source DataSource, incomin existing.PhysicalDisk.Serial, existing.PhysicalDisk.WWN, ) existingDevice := strings.ToLower(normalizePhysicalDiskDeviceToken(existing.PhysicalDisk.DevPath)) - agentReportsSAS := (source == SourceProxmox && strings.EqualFold(existing.PhysicalDisk.DiskType, "sas")) || - (source == SourceAgent && strings.EqualFold(incoming.PhysicalDisk.DiskType, "sas")) + // Proxmox may put a SAS disk's transport address in its serial, so + // the serials cannot refuse this path join, but a WWN naming another + // disk can: the agent's row may be the slot's previous occupant, + // retained by a silent agent, and joining it hands the replacement + // that disk's serial, readings and canonical resource. + agentReportsSAS := ((source == SourceProxmox && strings.EqualFold(existing.PhysicalDisk.DiskType, "sas")) || + (source == SourceAgent && strings.EqualFold(incoming.PhysicalDisk.DiskType, "sas"))) && + !diskinventory.HardwareIdentityConflict("", incomingWWN, "", existing.PhysicalDisk.WWN) deviceMatch := (agentReportsSAS || physicalDiskMissingIdentityPathCompatible(incoming.PhysicalDisk, existing.PhysicalDisk)) && incomingDevice != "" && incomingDevice == existingDevice && diff --git a/pkg/diskinventory/identity_conflict.go b/pkg/diskinventory/identity_conflict.go new file mode 100644 index 0000000000..32517cdc1b --- /dev/null +++ b/pkg/diskinventory/identity_conflict.go @@ -0,0 +1,61 @@ +package diskinventory + +import "strings" + +// HardwareIdentityConflict reports whether two observations of one disk slot +// carry hardware identities that name different disks: nothing agrees across +// serial and WWN (HardwareIdentityMatch), and at least one field reported on +// both sides disagrees. A field missing or unusable on either side is no +// evidence, so an observation without identity never conflicts. +// +// Reporters spell one identity differently, and spelling is not disagreement: +// - serials are compared on letters and digits only, because udev rewrites +// whitespace and unusual characters in a serial as underscores while +// smartctl keeps them, and a T10 vendor designator for an ATA drive +// ("ATA", the model, then the drive serial) is no evidence against the +// serial it ends with, because udev reports one when a SCSI-ATA +// translation layer hides IDENTIFY from it; +// - a WWN that begins with the other is no evidence, because udev's ID_WWN +// carries only the first 64 bits of a 128-bit NAA 6 identifier that sysfs +// and smartctl report in full. +// +// A caller blanks a field its producer may not report as disk identity, such +// as a SAS disk's serial, where Proxmox may report a SAS address, and checks +// HardwareIdentityMatch with the field kept if it may still show agreement. +func HardwareIdentityConflict(leftSerial, leftWWN, rightSerial, rightWWN string) bool { + if HardwareIdentityMatch(leftSerial, leftWWN, rightSerial, rightWWN) { + return false + } + return serialsDisagree(leftSerial, rightSerial) || wwnsDisagree(leftWWN, rightWWN) +} + +func serialsDisagree(left, right string) bool { + left, right = compactSerial(normalizeHardwareID(left)), compactSerial(normalizeHardwareID(right)) + return left != "" && right != "" && left != right && + !isATADesignatorOf(left, right) && !isATADesignatorOf(right, left) +} + +// isATADesignatorOf reports whether designator, compacted, is the T10 vendor +// designator of the ATA drive whose serial is given: the "ata" vendor field, +// a model, then that serial. +func isATADesignatorOf(designator, serial string) bool { + const vendor = "ata" + return len(designator) > len(vendor)+len(serial) && + strings.HasPrefix(designator, vendor) && strings.HasSuffix(designator, serial) +} + +func compactSerial(value string) string { + var builder strings.Builder + for _, char := range value { + if (char >= 'a' && char <= 'z') || (char >= '0' && char <= '9') { + builder.WriteRune(char) + } + } + return builder.String() +} + +func wwnsDisagree(left, right string) bool { + left, right = normalizeHardwareWWN(left), normalizeHardwareWWN(right) + return left != "" && right != "" && + !strings.HasPrefix(left, right) && !strings.HasPrefix(right, left) +} diff --git a/pkg/diskinventory/identity_conflict_test.go b/pkg/diskinventory/identity_conflict_test.go new file mode 100644 index 0000000000..ae84722881 --- /dev/null +++ b/pkg/diskinventory/identity_conflict_test.go @@ -0,0 +1,36 @@ +package diskinventory + +import "testing" + +func TestHardwareIdentityConflict(t *testing.T) { + for _, tc := range []struct { + name string + leftSerial, leftWWN string + rightSerial, rightWWN string + want bool + }{ + {name: "different serials", leftSerial: "WD-NEW0001", rightSerial: "WD-OLD0001", want: true}, + {name: "different WWNs", leftWWN: "0x5000c500b0000001", rightWWN: "naa.5000c500a0000001", want: true}, + {name: "WWN disagrees while serial is absent on one side", leftSerial: "PVE-ONLY", leftWWN: "0x5000c500b0000001", rightWWN: "5-c50-a0000001", want: true}, + {name: "same serial", leftSerial: "WD-WX1234", rightSerial: "wd-wx1234"}, + {name: "udev underscores for whitespace", leftSerial: "WD-WX_1234", rightSerial: "WD-WX 1234"}, + {name: "T10 designator ending with the drive serial", leftSerial: "ATA_ST4000NM000A-2HZ_ZC1ABCDE", rightSerial: "ZC1ABCDE"}, + {name: "serial that only begins with the other", leftSerial: "SN1234", rightSerial: "SN123", want: true}, + {name: "bare serial that ends with the other", leftSerial: "001234567890", rightSerial: "1234567890", want: true}, + {name: "agent NAA fields against PVE hex", leftSerial: "5000c500a0000001", leftWWN: "0x5000c500a0000001", rightSerial: "ZR5TESTA0001", rightWWN: "5-c50-a0000001"}, + {name: "udev 64-bit prefix of an NAA 6 WWN", leftWWN: "0x600508b1001c4d5e", rightWWN: "naa.600508b1001c4d5e7f8a9b0c1d2e3f40"}, + {name: "RAID volume NAA as serial against WWN", leftSerial: "600508b1001c4d5e7f8a9b0c1d2e3f40", leftWWN: "0x600508b1001c4d5e", rightSerial: "OTHER", rightWWN: "naa.600508b1001c4d5e7f8a9b0c1d2e3f40"}, + {name: "serial against WWN only is no evidence", leftSerial: "WD-NEW0001", rightWWN: "naa.5000c500a0000001"}, + {name: "placeholders are no evidence", leftSerial: "unknown", leftWWN: "0x0000000000000000", rightSerial: "WD-OLD0001", rightWWN: "naa.5000c500a0000001"}, + {name: "one side without identity", rightSerial: "WD-OLD0001", rightWWN: "naa.5000c500a0000001"}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := HardwareIdentityConflict(tc.leftSerial, tc.leftWWN, tc.rightSerial, tc.rightWWN); got != tc.want { + t.Fatalf("HardwareIdentityConflict = %v, want %v", got, tc.want) + } + if got := HardwareIdentityConflict(tc.rightSerial, tc.rightWWN, tc.leftSerial, tc.leftWWN); got != tc.want { + t.Fatalf("HardwareIdentityConflict (swapped) = %v, want %v", got, tc.want) + } + }) + } +} From e02edd9989c88fbe6e5459afdf488c73538ddd24 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 12:29:01 +0100 Subject: [PATCH 08/12] Show retained disk temperatures as last known in drawers and tables Normalization keeps a disk's last temperature when the current observation did not collect one, such as a disk in standby or a host agent past its reporting lease. PR #2603 taught the Storage presenters to show that value as last known. Four surfaces still read the raw number as a current reading: - The TrueNAS disk drawer's Temperature row took the 55C warning tone from a retained reading, and its summary listed it as a reading. The row now says "(last known)" with the reason as title and no tone. The summary drops it because it has no room for the reason. - The drawer History fallback fed a retained reading into smart_temp and labelled it "current" when no points were stored. It now goes through the deferred-metric slot, so the legend says "last known" and the reason shows above the chart. The disk's value is read from physicalDisk beside its collection state, because websocket rows carry no top-level temperature for a disk and showed "-" instead. - The TrueNAS storage table's Temp cell showed a retained reading plainly, and hottest-first sorted it to the top. It now renders muted with its reason, and the sort ranks it as no reading. - The Proxmox outdated sensor setup notice counted a retained reading as a temperature arriving, so it could hide for a SATA disk Pulse cannot read now. All four reuse getPhysicalDiskTemperaturePresentation or isPhysicalDiskTemperatureCurrent instead of adding a predicate. --- .../subsystems/frontend-primitives.md | 17 +++++ .../internal/subsystems/unified-resources.md | 7 +++ .../294409f678589ef2.json | 52 +++++++++++++++ ...rMetricsHistoryModel.branchcov0712.test.ts | 60 ++++++++++++++++++ .../resourceDetailDrawerTrueNASModel.test.ts | 37 +++++++++++ ...resourceDetailDrawerMetricsHistoryModel.ts | 33 +++++++++- .../resourceDetailDrawerTrueNASModel.ts | 42 ++++++++++--- .../features/platformPage/sensorSetup.test.ts | 26 +++++++- .../src/features/platformPage/sensorSetup.ts | 5 +- .../truenas/TrueNASStorageTopologyTable.tsx | 54 ++++++++++++---- .../TrueNASStorageTopologyTable.test.tsx | 63 +++++++++++++++++++ 11 files changed, 373 insertions(+), 23 deletions(-) create mode 100644 frontend-modern/browser-verification/294409f678589ef2.json diff --git a/docs/release-control/v6/internal/subsystems/frontend-primitives.md b/docs/release-control/v6/internal/subsystems/frontend-primitives.md index bf1638080e..d837f96e3d 100644 --- a/docs/release-control/v6/internal/subsystems/frontend-primitives.md +++ b/docs/release-control/v6/internal/subsystems/frontend-primitives.md @@ -7883,6 +7883,23 @@ When the cell's value is a retained host-agent SMART temperature, it renders muted with a dotted underline and screen-reader "last known", as does a retained value in the guest card. Tooltip and Thermals rows say "(last known)", and the Thermals row carries the collection reason as its title. +The shared drawer History reads the same decision. For a physical disk, +`resourceDetailDrawerMetricsHistoryModel.ts` takes the reading from +`physicalDisk.temperature`, beside the collection state that qualifies it, +because websocket rows carry no top-level `temperature` for a disk. When that +state is set and is not `available` (an empty or absent state predates the +contract and stays current), `getResourceMetricsHistoryCurrentMetrics` offers +no current `smart_temp`. A positive retained reading then reaches +`getResourceMetricsHistoryDeferredMetrics` as a deferred metric: with no +history points the legend says "last known", never "current", and the reason +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 focused browser proofs are `frontend-modern/src/features/patrol/__tests__/patrolRunAcceptance.test.ts`, diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index bc3d781246..25fb124809 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -1122,6 +1122,13 @@ for the Physical Disks verdict. The heat reason follows any native TrueNAS alert text, and a hot disk's status dot turns warning though its source state stays `online`, because phones show only the dot. `truenasPageModel.test.ts` and `TrueNASStorageTopologyTable.test.tsx` pin it. +The Temp column reads the same collection state through +`getPhysicalDiskTemperaturePresentation`. A disk temperature that is not +current, such as a host agent's last reading kept on a merged disk row after +the agent stopped reporting, renders as last known: muted, with its reason as +the title and screen-reader "last known" text. The Temp sort ranks it as no +reading, and the tree sort orders each sibling group, so a retained value +never leads its group hottest first. The vSphere Datastores and Networks tables follow the same exception-first Health column. `getVmwareDatastoreIssue` and `getVmwareNetworkIssue` return nothing for a green row; otherwise the reasons are, in order, an impaired diff --git a/frontend-modern/browser-verification/294409f678589ef2.json b/frontend-modern/browser-verification/294409f678589ef2.json new file mode 100644 index 0000000000..726e7fa021 --- /dev/null +++ b/frontend-modern/browser-verification/294409f678589ef2.json @@ -0,0 +1,52 @@ +{ + "version": 1, + "base_sha": "41cea2ffab5ca2d39240d14138975bc4e902bb61", + "verified_at": "2026-10-07T11:28:58Z", + "result": "passed", + "changed_paths": [ + "frontend-modern/src/components/Infrastructure/resourceDetailDrawerMetricsHistoryModel.ts", + "frontend-modern/src/components/Infrastructure/resourceDetailDrawerTrueNASModel.ts", + "frontend-modern/src/features/platformPage/sensorSetup.ts", + "frontend-modern/src/features/truenas/TrueNASStorageTopologyTable.tsx" + ], + "content_sha256": { + "frontend-modern/src/components/Infrastructure/resourceDetailDrawerMetricsHistoryModel.ts": "e6e8b46a156df4a1d1b391de19f041c0a1f74eb7e2ecdb78fdfa773a4c336262", + "frontend-modern/src/components/Infrastructure/resourceDetailDrawerTrueNASModel.ts": "fa1c743eaec09f424c80aa5d3aee9082817e54353a853b542620eb45792f5887", + "frontend-modern/src/features/platformPage/sensorSetup.ts": "5b92f1fc07b513a11f0e8f2654b0717baa4b99d742eae76368844731a5f89eb9", + "frontend-modern/src/features/truenas/TrueNASStorageTopologyTable.tsx": "dd7c5189a950adba606a7ea4d0d40233d158e965883135b6fe82b772a6dcf5cd" + }, + "routes": [ + "/truenas/storage?kind=disks", + "/proxmox/storage", + "/proxmox" + ], + "viewports": [ + { + "width": 1440, + "height": 900 + }, + { + "width": 390, + "height": 844 + } + ], + "states": [ + "Vite dev server on this tree (origin/main 41cea2ffab plus the change) against a private PULSE_MOCK_MODE backend (8-node estate) built with a local-only go build -overlay (never committed). The overlay stamps TrueNAS disk sdb with physicalDisk.temperature 61 and collection.temperature unavailable 'host agent stopped reporting' (the shape a TrueNAS disk merged with a silent host agent row reaches), pins sdd to a current 58, gives pve3 SATA disk /dev/nvme2 the same retained state (51) and marks pve3's sensor payload legacySensorsFormat; /api/resources and the websocket rows carried those states", + "Before control in the same session (the four runtime files swapped to origin/main under the running Vite, then restored and compared byte for byte): sdb Temp cell plain 61.0°C sorting first in hottest-first, drawer Temperature row 61°C in the amber warning tone, TrueNAS summary 'SATA, Passed, 16.0 TB, 61°C', History with no smart_temp points showing '-', and no outdated sensor setup notice for pve3", + "After, TrueNAS storage disks view: sdb Temp cell '61.0°C' muted (rgb 71,85,105) with a dotted underline, title 'Last known reading, not current: host agent stopped reporting', data-temperature-reading=last-known and screen-reader ', last known'; current disks (sdd 58.0°C, sda 46.0°C) unchanged; no Attention reason for sdb", + "After, Temp sort: descending sdd 58, sdf 47, nvme0n1 46, sda 46, sde 42, nvme1n1 41, sdc 39, then sdb last; ascending sdc 39 ... sdd 58, then sdb last (a retained reading sorts as no reading)", + "After, sdb row drawer (Platform details > Show TrueNAS): Health Temperature row '61°C (last known)' in text-muted with the reason as its title; the collapsed TrueNAS summary reads 'SATA, Passed, 16.0 TB' with no temperature", + "After, sdb drawer History tab: with seeded points the legend shows the stored 53°C and the line 'Disk reading: Last known reading, not current: host agent stopped reporting.'; with the smart_temp series emptied by a Playwright route the legend reads 'Disk 61°C last known' (data-history-last-known=smart_temp, no 'current') with the same line; current disk sdd with no points reads 'Disk 58°C current'", + "After, /proxmox/storage: 'pve3 is using an older temperature monitoring setup that cannot read SATA/SAS disk temperatures. Re-run the node setup script to upgrade it.' notice shows at both widths (hidden on main); /proxmox Overview loads no physical disks so the notice cannot show there on main or after (pre-existing, chipped separately)", + "No page errors at 1440x900 or 390x844", + "Re-run after review edits (History model reads a disk's value only from physicalDisk, never a top-level fallback): every reading above was unchanged at both widths" + ], + "interactions": [ + "Opened /truenas/storage?kind=disks at 1440x900 and 390x844 and read every disk row's Temp cell (text, title, data attribute, computed colour and decoration)", + "Clicked the Temp column header twice at 1440x900 and read the disk row order for descending and ascending", + "Opened the sdb row drawer with 'Expand details for sdb', opened Platform details and clicked 'Show TrueNAS', read the summary and the Health Temperature row at both widths", + "Opened the drawer History tab with stored points, returned to Overview, then reopened History with the smart_temp series emptied, and read the Temperature legend and reason line at both widths; repeated for current disk sdd", + "Opened /proxmox and /proxmox/storage at both widths and read the outdated sensor setup notice", + "Ran the same Playwright pass against the origin/main runtime files as the before control, restored the change and confirmed the full git diff was byte-identical" + ] +} diff --git a/frontend-modern/src/components/Infrastructure/__tests__/resourceDetailDrawerMetricsHistoryModel.branchcov0712.test.ts b/frontend-modern/src/components/Infrastructure/__tests__/resourceDetailDrawerMetricsHistoryModel.branchcov0712.test.ts index bae3d26c0e..9fe9a0ac3b 100644 --- a/frontend-modern/src/components/Infrastructure/__tests__/resourceDetailDrawerMetricsHistoryModel.branchcov0712.test.ts +++ b/frontend-modern/src/components/Infrastructure/__tests__/resourceDetailDrawerMetricsHistoryModel.branchcov0712.test.ts @@ -513,6 +513,66 @@ describe('getResourceMetricsHistoryCurrentMetrics branch coverage', () => { }); }); + it('offers a retained physical disk temperature only as last known, with its reason', () => { + // useUnifiedResources copies physicalDisk.temperature onto the resource. + // A disk in standby keeps its last reading under an unavailable state, so + // with no history points the chart must not label it current. + const disk = (state?: 'available' | 'unavailable', temperature = 47) => + baseResource({ + type: 'physical_disk', + metricsTarget: { resourceType: 'disk', resourceId: 'SERIAL-1' }, + temperature, + physicalDisk: { + devPath: '/dev/sdb', + temperature, + collection: state + ? { temperature: { state, source: 'smartctl', reason: 'disk is in standby' } } + : undefined, + }, + }); + + expect(getResourceMetricsHistoryCurrentMetrics(disk('unavailable'))).toMatchObject({ + smart_temp: undefined, + temperature: undefined, + }); + expect(getResourceMetricsHistoryDeferredMetrics(disk('unavailable')).smart_temp).toEqual({ + lastKnownValue: 47, + message: 'Last known reading, not current: disk is in standby.', + }); + // An unavailable state with no retained reading has nothing to offer. + expect(getResourceMetricsHistoryCurrentMetrics(disk('unavailable', 0)).smart_temp).toBe( + undefined, + ); + expect(getResourceMetricsHistoryDeferredMetrics(disk('unavailable', 0)).smart_temp).toBe( + undefined, + ); + + for (const current of [disk('available'), disk()]) { + expect(getResourceMetricsHistoryCurrentMetrics(current).smart_temp).toBe(47); + expect(getResourceMetricsHistoryDeferredMetrics(current).smart_temp).toBeUndefined(); + } + + // Websocket rows carry the reading only on physicalDisk, beside its state. + const websocketRow = (state: 'available' | 'unavailable') => { + const row = disk(state); + delete row.temperature; + return row; + }; + expect(getResourceMetricsHistoryCurrentMetrics(websocketRow('available')).smart_temp).toBe(47); + expect( + getResourceMetricsHistoryCurrentMetrics(websocketRow('unavailable')).smart_temp, + ).toBeUndefined(); + expect( + getResourceMetricsHistoryDeferredMetrics(websocketRow('unavailable')).smart_temp, + ).toMatchObject({ lastKnownValue: 47 }); + + // A top-level value never stands in for the disk's own missing reading, + // which the disk's collection state could not qualify. + const unpaired = disk('available'); + delete unpaired.physicalDisk!.temperature; + expect(getResourceMetricsHistoryCurrentMetrics(unpaired).smart_temp).toBeUndefined(); + }); + it('uses the source-aware host subset for API-only Proxmox node targets', () => { const resource = baseResource({ type: 'agent', diff --git a/frontend-modern/src/components/Infrastructure/__tests__/resourceDetailDrawerTrueNASModel.test.ts b/frontend-modern/src/components/Infrastructure/__tests__/resourceDetailDrawerTrueNASModel.test.ts index 539aee7b89..1afd532170 100644 --- a/frontend-modern/src/components/Infrastructure/__tests__/resourceDetailDrawerTrueNASModel.test.ts +++ b/frontend-modern/src/components/Infrastructure/__tests__/resourceDetailDrawerTrueNASModel.test.ts @@ -361,6 +361,43 @@ describe('resourceDetailDrawerTrueNASModel', () => { ]); }); + it('shows a retained disk temperature as last known, with its reason and no heat tone', () => { + // A TrueNAS disk merged with a host agent's row keeps the agent's last + // reading after the agent stops reporting. It is not current, so it must + // not read as a disk running hot now, nor sit in the summary as a reading. + const disk = (state: 'available' | 'unavailable') => + baseResource({ + type: 'physical_disk', + displayName: 'sdc', + platformScopes: ['truenas'], + physicalDisk: { + devPath: '/dev/sdc', + diskType: 'sata', + sizeBytes: 24 * 1024 ** 4, + health: 'PASSED', + temperature: 61, + collection: { + temperature: { state, source: 'agent', reason: 'host agent stopped reporting' }, + }, + }, + }); + const temperatureRow = (resource: Resource) => + buildTrueNASDetailSections(resource) + .find((section) => section.label === 'Health') + ?.rows.find((row) => row.label === 'Temperature'); + + expect(temperatureRow(disk('unavailable'))).toMatchObject({ + value: '61°C (last known)', + tone: 'muted', + title: 'Last known reading, not current: host agent stopped reporting', + }); + expect(buildTrueNASDetailsSummary(disk('unavailable'))).toBe('SATA, Passed, 24.0 TB'); + + expect(temperatureRow(disk('available'))).toMatchObject({ value: '61°C', tone: 'warning' }); + expect(temperatureRow(disk('available'))?.title).toBeUndefined(); + expect(buildTrueNASDetailsSummary(disk('available'))).toBe('SATA, Passed, 24.0 TB, 61°C'); + }); + it('shows the vdev layout as the storage kind while topology stays the pool discriminator', () => { // Regression guard for 599c8e634. `topology` is the cross-provider // discriminator ('pool' | 'dataset' | ...); the ZFS layout belongs in diff --git a/frontend-modern/src/components/Infrastructure/resourceDetailDrawerMetricsHistoryModel.ts b/frontend-modern/src/components/Infrastructure/resourceDetailDrawerMetricsHistoryModel.ts index 2fc02a3e4c..8d4ee55a4b 100644 --- a/frontend-modern/src/components/Infrastructure/resourceDetailDrawerMetricsHistoryModel.ts +++ b/frontend-modern/src/components/Infrastructure/resourceDetailDrawerMetricsHistoryModel.ts @@ -14,6 +14,10 @@ import { import type { HostGPUSensor } from '@/types/api'; import type { Resource } from '@/types/resource'; import { getDiskPercent, getMemoryPercent } from '@/types/resource'; +import { + getPhysicalDiskTemperaturePresentation, + isPhysicalDiskTemperatureCurrent, +} from '@/features/storageBackups/diskTemperaturePresentation'; import { asTrimmedString } from '@/utils/stringUtils'; import { getMemoryObservationPresentation } from '@/utils/memoryObservation'; import { getWorkloadGuestDiskStatusMessage } from '@/utils/workloadGuestPresentation'; @@ -174,6 +178,18 @@ const diskReadReason = (resource: Resource): string | undefined => ? resource.proxmox?.diskStatusReason || undefined : undefined; +// A physical disk's reading and its collection state both live on +// physicalDisk, so read them only together: websocket rows carry no top-level +// temperature for a disk, which only the REST mapper copies across. The +// reading may be a last known one kept while the current observation could +// not collect it, such as a disk in standby or a host agent past its +// reporting lease, and is then never current. +const resourceTemperature = (resource: Resource): number | undefined => + resource.physicalDisk ? resource.physicalDisk.temperature : resource.temperature; + +const isResourceTemperatureCurrent = (resource: Resource): boolean => + isPhysicalDiskTemperatureCurrent(resource.physicalDisk?.collection); + export const getResourceMetricsHistoryDeferredMetrics = ( resource: Resource, ): Record => { @@ -197,6 +213,19 @@ export const getResourceMetricsHistoryDeferredMetrics = ( message: getWorkloadGuestDiskStatusMessage(reason), }; } + if (!isResourceTemperatureCurrent(resource)) { + const temperature = resourceTemperature(resource); + const retained = getPhysicalDiskTemperaturePresentation({ + temperature: temperature ?? 0, + collection: resource.physicalDisk?.collection, + }); + if (retained?.title) { + metrics.smart_temp = { + lastKnownValue: temperature, + message: `${retained.title}.`, + }; + } + } return metrics; }; @@ -204,7 +233,9 @@ export const getResourceMetricsHistoryCurrentMetrics = ( resource: Resource, ): Record => { const diskPercent = resource.disk ? finiteMetric(getDiskPercent(resource)) : undefined; - const temperature = finiteMetric(resource.temperature); + const temperature = isResourceTemperatureCurrent(resource) + ? finiteMetric(resourceTemperature(resource)) + : undefined; const memory = memoryReading(resource); return { cpu: finiteMetric(resource.cpu?.current), diff --git a/frontend-modern/src/components/Infrastructure/resourceDetailDrawerTrueNASModel.ts b/frontend-modern/src/components/Infrastructure/resourceDetailDrawerTrueNASModel.ts index f8a666aa5d..57d3fca8c9 100644 --- a/frontend-modern/src/components/Infrastructure/resourceDetailDrawerTrueNASModel.ts +++ b/frontend-modern/src/components/Infrastructure/resourceDetailDrawerTrueNASModel.ts @@ -20,6 +20,7 @@ import { type DetailSection, type DetailValueTone, } from '@/components/shared/detailSectionModel'; +import { getPhysicalDiskTemperaturePresentation } from '@/features/storageBackups/diskTemperaturePresentation'; import { hasImpairedResourceSource } from '@/utils/resourceSourceHealth'; export type ResourceDetailDrawerTrueNASRowTone = DetailValueTone; @@ -51,11 +52,6 @@ const formatPercent = (percent?: number): string | null => { return `${percent.toFixed(percent >= 10 ? 1 : 2)}%`; }; -const formatTemperature = (celsius?: number): string | null => { - if (typeof celsius !== 'number' || !Number.isFinite(celsius) || celsius <= 0) return null; - return `${celsius.toFixed(0)}°C`; -}; - const formatDurationSeconds = (seconds?: number): string | null => { const value = asPositiveNumber(seconds); if (!value) return null; @@ -396,6 +392,35 @@ const formatDiskHours = (hours?: number): string | null => { return `${formatDetailIntegerValue(value) ?? value.toFixed(0)}h`; }; +const diskTemperature = (disk: ResourcePhysicalDiskMeta) => + getPhysicalDiskTemperaturePresentation({ + temperature: disk.temperature ?? 0, + collection: disk.collection, + }); + +// A retained reading, such as a silent host agent's on a merged disk row, is +// not current: it takes no heat tone and its title says why. +const diskTemperatureRow = ( + disk: ResourcePhysicalDiskMeta, +): ResourceDetailDrawerTrueNASRow | null => { + const reading = diskTemperature(disk); + if (!reading) return null; + if (!reading.current) { + return row('Temperature', `${reading.label} (last known)`, { + title: reading.title, + tone: 'muted', + }); + } + return row('Temperature', reading.label, { + tone: (disk.temperature ?? 0) >= 55 ? 'warning' : 'default', + }); +}; + +const currentDiskTemperatureLabel = (disk: ResourcePhysicalDiskMeta): string | null => { + const reading = diskTemperature(disk); + return reading?.current ? reading.label : null; +}; + const buildTrueNASDiskSections = ( resource: Resource, disk: ResourcePhysicalDiskMeta, @@ -414,9 +439,7 @@ const buildTrueNASDiskSections = ( const healthRows = compactRows([ row('Health', normalizeDelimitedLabel(disk.health), { tone: diskStateTone(disk) }), - row('Temperature', formatTemperature(disk.temperature), { - tone: disk.temperature && disk.temperature >= 55 ? 'warning' : 'default', - }), + diskTemperatureRow(disk), row( 'Wearout', disk.wearout === undefined || disk.wearout < 0 ? null : formatPercent(disk.wearout), @@ -795,7 +818,8 @@ export const buildTrueNASDetailsSummary = (resource: Resource): string | null => diskTypeLabel(disk.diskType), normalizeDelimitedLabel(disk.health), formatDetailBytesValue(disk.sizeBytes), - formatTemperature(disk.temperature), + // The summary has no room to say a retained reading is not current. + currentDiskTemperatureLabel(disk), ].filter((value): value is string => Boolean(value)); return summary.length > 0 ? summary.join(', ') : null; } diff --git a/frontend-modern/src/features/platformPage/sensorSetup.test.ts b/frontend-modern/src/features/platformPage/sensorSetup.test.ts index 9d42cbecc9..3b2ada61d9 100644 --- a/frontend-modern/src/features/platformPage/sensorSetup.test.ts +++ b/frontend-modern/src/features/platformPage/sensorSetup.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest'; -import type { Resource } from '@/types/resource'; +import type { PhysicalDiskCollectionStatus, Resource } from '@/types/resource'; import { collectOutdatedSensorSetupNodes } from './sensorSetup'; const node = (over: Partial & { proxmox?: Resource['proxmox'] }): Resource => @@ -11,6 +11,7 @@ const disk = (over: { nodeName?: string; diskType?: string; temperature?: number; + collection?: PhysicalDiskCollectionStatus; }): Resource => ({ id: over.id ?? 'disk-1', @@ -20,6 +21,7 @@ const disk = (over: { physicalDisk: { diskType: over.diskType ?? 'sata', temperature: over.temperature ?? 0, + collection: over.collection, }, platformData: over.nodeName ? { proxmox: { nodeName: over.nodeName } } : {}, }) as unknown as Resource; @@ -80,6 +82,28 @@ describe('collectOutdatedSensorSetupNodes', () => { ).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' }, + ]); + 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( diff --git a/frontend-modern/src/features/platformPage/sensorSetup.ts b/frontend-modern/src/features/platformPage/sensorSetup.ts index 04c1378370..af8ff285b2 100644 --- a/frontend-modern/src/features/platformPage/sensorSetup.ts +++ b/frontend-modern/src/features/platformPage/sensorSetup.ts @@ -1,5 +1,6 @@ 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. @@ -18,7 +19,9 @@ const isSMARTOnlyDiskWithoutTemperature = (disk: Resource): boolean => { if (!meta) return false; const diskType = (meta.diskType || '').trim().toLowerCase(); if (!SMART_ONLY_DISK_TYPES.has(diskType)) return false; - return (meta.temperature ?? 0) <= 0; + // 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 diff --git a/frontend-modern/src/features/truenas/TrueNASStorageTopologyTable.tsx b/frontend-modern/src/features/truenas/TrueNASStorageTopologyTable.tsx index 27c9caf59a..baa629330f 100644 --- a/frontend-modern/src/features/truenas/TrueNASStorageTopologyTable.tsx +++ b/frontend-modern/src/features/truenas/TrueNASStorageTopologyTable.tsx @@ -37,6 +37,10 @@ import { createPlatformResourceLabelResolver, getPlatformResourceDetailRowClass, } from '@/features/platformPage/PlatformResourceDetailTableRow'; +import { + PHYSICAL_DISK_TEMPERATURE_LAST_KNOWN_CLASS, + getPhysicalDiskTemperaturePresentation, +} from '@/features/storageBackups/diskTemperaturePresentation'; import { useAlertsActivation } from '@/stores/alertsActivation'; import type { Resource } from '@/types/resource'; import { @@ -200,6 +204,40 @@ const DiskEnduranceCell: Component<{ row: TrueNASStorageTopologyRow }> = (props) ); }; +// A disk row may carry a last known temperature it did not collect now, such +// as a host agent's reading after the agent stopped reporting on a merged disk. +const diskTemperatureReading = (row: TrueNASStorageTopologyRow) => { + const disk = row.kind === 'disk' ? row.resource.physicalDisk : undefined; + return disk + ? getPhysicalDiskTemperaturePresentation({ + temperature: disk.temperature ?? 0, + collection: disk.collection, + }) + : null; +}; + +// A retained reading reads as last known, never as the disk's temperature now. +const DiskTemperatureCell: Component<{ row: TrueNASStorageTopologyRow }> = (props) => { + const reading = () => diskTemperatureReading(props.row); + const value = () => + props.row.kind === 'disk' ? props.row.resource.physicalDisk?.temperature : undefined; + return ( + } + > + + + , last known + + + ); +}; + const ISSUE_TONES: Record = { attention: 'warning', offline: 'danger', @@ -253,9 +291,10 @@ const getTrueNASStorageSortValue = ( case 'endurance': return row.kind === 'disk' ? (diskEndurance(row)?.value ?? null) : null; case 'temp': { - if (row.kind !== 'disk') return null; - const temperature = row.resource.physicalDisk?.temperature; - return typeof temperature === 'number' && Number.isFinite(temperature) ? temperature : null; + // Hottest first ranks current readings only; a last known one sorts as + // no reading. + const reading = diskTemperatureReading(row); + return reading?.current ? (row.resource.physicalDisk?.temperature ?? null) : null; } case 'health': return HEALTH_SORT_RANK[ @@ -575,14 +614,7 @@ export const TrueNASStorageTopologyTable: Component<{ kindFilter() === 'disks' ? 'table-cell' : 'hidden lg:table-cell' } text-base-content`} > - +     { ).toEqual(['sda', 'nvme0n1']); }); + it('shows a retained disk temperature as last known and sorts it as no reading', async () => { + // A TrueNAS disk merged with a host agent's row keeps the agent's last + // reading after the agent stops reporting, under an unavailable state. + const disk = (id: string, temperature: number, state: 'available' | 'unavailable') => + makeStorageResource({ + id, + type: 'physical_disk', + name: id, + storage: undefined, + physicalDisk: { + devPath: `/dev/${id}`, + serial: `serial-${id}`, + diskType: 'sata', + temperature, + collection: { + temperature: { state, source: 'agent', reason: 'host agent stopped reporting' }, + }, + }, + }); + const resources = [ + disk('sda', 64, 'unavailable'), + disk('sdb', 41, 'available'), + disk('sdc', 38, 'available'), + ]; + const { container } = render(() => ( + } + emptyTitle="No storage" + emptyDescription="No storage" + showToolbar={false} + /> + )); + const row = (id: string) => container.querySelector(`[data-truenas-storage-resource="${id}"]`); + const order = () => + [...container.querySelectorAll('[data-truenas-storage-resource]')].map((element) => + element.getAttribute('data-truenas-storage-resource'), + ); + + try { + const retained = row('sda')?.querySelector('[data-temperature-reading="last-known"]'); + expect(retained).toHaveTextContent('64.0°C, last known'); + expect(retained).toHaveAttribute( + 'title', + 'Last known reading, not current: host agent stopped reporting', + ); + expect(retained).toHaveClass('text-muted'); + // 64C is over the factory SATA trigger, but a retained reading is not heat. + expect(row('sda')?.querySelector('[data-truenas-storage-health]')).toBeNull(); + expect(row('sdb')?.querySelector('[data-temperature-reading="last-known"]')).toBeNull(); + expect(row('sdb')).toHaveTextContent('41.0°C'); + + // Hottest first ranks current readings only; the retained one stays last. + await fireEvent.click(screen.getByRole('columnheader', { name: /Temp/ })); + expect(order()).toEqual(['sdb', 'sdc', 'sda']); + await fireEvent.click(screen.getByRole('columnheader', { name: /Temp/ })); + expect(order()).toEqual(['sdc', 'sdb', 'sda']); + } finally { + window.localStorage.clear(); + } + }); + it('follows a disk temperature trigger the user raised in Alerts', async () => { const getConfig = vi.spyOn(AlertsAPI, 'getConfig').mockResolvedValue({ enabled: true, From 425dd0d45b73a7effcee6e3ee3f882b1c899eb3a Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 12:56:47 +0100 Subject: [PATCH 09/12] Keep same-serial disks on different machines apart findMatch refuses a physical-disk candidate under another parent, but ingestRecord then minted the same hardware-keyed canonical ID from the serial and merged into its holder on the collision. Disks that report one usable serial on several machines (dual-ported SAS shelves, cloned VMs with an explicit serial, fixed-serial USB bridges, TrueNAS systems) collapsed into one resource parented to whichever machine ingested last, and every other machine lost the disk from inventory. A new disk now resolves its parent to a machine (the nearest host ancestor, so pools and Unraid arrays count as their host) and stays apart only on evidence: one reporter placing the serial on two machines, or machines whose hostnames share nothing. Reporters on one box still join across grouping parents, so the TrueNAS API and the agent on that box stay one disk, and an unknown machine never splits a disk. Once a serial spans machines, every copy takes the serial keyed to its own machine and the unscoped holder is re-keyed, so IDs do not depend on machine ingest order and the unscoped ID never passes between disks that coexist. A disk only one machine reports keeps its ID. Operator splits now hold on this path; the old collision merge ignored them. The registry indexes disks by hardware key because the identity matcher keeps one resource per machine ID. --- .../internal/subsystems/unified-resources.md | 42 ++ internal/unifiedresources/registry.go | 273 +++++++++++ .../registry_merge_policy_test.go | 462 ++++++++++++++++++ 3 files changed, 777 insertions(+) diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index 6f82da2f6d..957af34194 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -5325,6 +5325,48 @@ serial or WWN only inside a compatible physical-disk parent scope; when those identifiers are absent or placeholders, host/device/topology fallback identity is authoritative. An exact seeded source mapping may be reused after restart only when its resource type and parent scope still agree. +A usable serial or WWN identifies a disk on one machine only, and the canonical +ID a new disk takes must honour that, not just matching. Dual-ported SAS +shelves, cloned VMs with an explicit serial, fixed-serial USB bridges and +TrueNAS systems report one identifier on several machines, and merging them on +a hardware-keyed ID collision dropped every machine but the last from +inventory. `physicalDiskIDForMachineLocked` resolves each disk's parent to its +machine (the nearest host ancestor, so a TrueNAS pool or Unraid array counts as +its system or host) and treats two disks carrying one identity as different +machines only on evidence: one reporter placing the identity on two machines +(for a single-source disk rehydrated without per-source parents, its own +parent), or differing machines whose hostnames share nothing. Reporters on one +machine still join across grouping parents and unmerged host resources (the +TrueNAS API and the agent on that box), and an unknown machine never splits a +disk. A split (operator exclusion) recorded against any ID the disk can hold, +unscoped, current or machine-scoped, keeps the observation on its +source-specific ID, keyed to its machine when another machine's disk already +holds that ID (agent disk source IDs are the bare serial). Once the identity +spans machines the unscoped ID no longer names one disk, so a split recorded +against it applies to every copy, erring towards an extra row rather than a +merge the operator forbade, and a manual link recorded against it stops +applying. More than one joinable disk joins none, except that the holder of the +unscoped ID keeps it. Once an identity spans machines, every copy not split off +onto its source-specific ID takes the identity keyed to the machine of its +canonical parent at that point, and the holder of the unscoped ID is re-keyed +with its source mappings, retired-ID claims and relationship endpoints. +Machines may be ingested in any order, and the unscoped ID never passes between +disks that coexist on different machines. A disk only one machine reports keeps +its unscoped ID, apart from observations an operator split off. Residuals: like +any serial-keyed ID, the unscoped ID still passes to a same-serial disk that +appears once the previous holder is gone; a disk's ID changes when a +same-serial disk first appears on another machine and back when it goes away, +and alert-history rows owned under the unscoped ID do not follow a re-key; a +disk two reporters share is scoped to its canonical parent when the identity +first spans machines, which the fixed ingest order (snapshot sources, then +supplemental records) and source priority decide; agent SMART and Unraid disk +source IDs are the bare serial, so same-serial agent disks on different hosts +share one `SourceAgent` mapping and only the last ingested keeps the agent +source target; same-serial disks share one metrics history key; and across +reporters, hostnames that normalize alike (default TrueNAS names, short names +across domains) still join. `IdentityMatcher` keeps one resource per machine +ID, so the registry indexes disks by hardware key (`physicalDisksByHardware`) +to see every copy. That same canonical physical-disk view must also expose source-independent host context. When a disk is API-backed rather than node-backed, typed views should fall back to canonical host identity such as `identity.hostnames` instead of diff --git a/internal/unifiedresources/registry.go b/internal/unifiedresources/registry.go index 6c9a786cad..afc35b3e21 100644 --- a/internal/unifiedresources/registry.go +++ b/internal/unifiedresources/registry.go @@ -104,6 +104,11 @@ type ResourceRegistry struct { // nil during a batch means reference reads use the live scan until the // final identity refresh rebuilds the index. Empty values are ambiguous. canonicalIdentityIndex map[string]string + // physicalDisksByHardware lists every disk resource ever indexed under a + // hardware key (physicalDiskHardwareKey). The IdentityMatcher keeps one + // resource per machine ID, so it cannot see same-serial disks on several + // machines. Entries are validated on read, never removed eagerly. + physicalDisksByHardware map[string]map[string]struct{} // Cached typed view indexes. Invalidated on ingest, rebuilt lazily on // first access. Protected by mu — callers hold RLock to read, and the @@ -835,6 +840,7 @@ func (rr *ResourceRegistry) ingestResources(resources []Resource, thresholds map rr.seedSourceMappingsFromResourceLocked(rr.resources[resourceID]) if resource := rr.resources[resourceID]; resource != nil { rr.indexSupersededCanonicalIDsLocked(resourceID, resource.SupersededCanonicalIDs) + rr.indexPhysicalDiskHardwareLocked(resource) } } rr.applyManualLinks(thresholds) @@ -2929,6 +2935,7 @@ func (rr *ResourceRegistry) ingestRecord(source DataSource, sourceID string, res } stampPhysicalDiskTemperatureReading(&resource, source, sourceID) rr.resources[resource.ID] = &resource + rr.indexPhysicalDiskHardwareLocked(&resource) rr.bySource[source][sourceID] = resource.ID rr.matcher.Add(resource.ID, identity) return resource.ID @@ -2936,6 +2943,9 @@ func (rr *ResourceRegistry) ingestRecord(source DataSource, sourceID string, res } resource.ID = rr.chooseNewID(resource.Type, identity, source, sourceID) + if resource.Type == ResourceTypePhysicalDisk { + resource.ID = rr.physicalDiskIDForMachineLocked(resource.ID, candidateID, &resource, source, onlyMissing) + } normalizeResourceRelationships(&resource) if existing := rr.resources[resource.ID]; existing != nil { if onlyMissing { @@ -2948,6 +2958,7 @@ func (rr *ResourceRegistry) ingestRecord(source DataSource, sourceID string, res } stampPhysicalDiskTemperatureReading(&resource, source, sourceID) rr.resources[resource.ID] = &resource + rr.indexPhysicalDiskHardwareLocked(&resource) rr.bySource[source][sourceID] = resource.ID rr.matcher.Add(resource.ID, identity) @@ -3836,6 +3847,7 @@ func (rr *ResourceRegistry) mergeInto(existing *Resource, incoming Resource, sou existing.Name = incoming.Name } } + rr.indexPhysicalDiskHardwareLocked(existing) } func clearUnavailableSourceMemoryMetric(metrics *ResourceMetrics, incoming *Resource, source DataSource) *ResourceMetrics { @@ -5260,6 +5272,267 @@ func (rr *ResourceRegistry) chooseNewID(resourceType ResourceType, identity Reso return rr.sourceSpecificID(resourceType, source, sourceID) } +// physicalDiskIDForMachineLocked decides which resource a new disk +// observation joins once findMatch has refused every candidate. A usable +// serial or WWN identifies a disk only on one machine: dual-ported SAS shelves, +// cloned VMs with an explicit serial, fixed-serial USB bridges and TrueNAS +// systems report one identifier on several machines. id is chooseNewID's +// hardware-keyed ID and candidateID the observation's source-specific ID. +// +// With no other disk carrying the identity, the disk keeps id, so a disk seen +// on one machine never changes ID. Otherwise it joins the one disk it does not +// conflict with (a second reporter on the same machine), unless an operator +// exclusion separates them, which leaves it on candidateID as findMatch's +// excluded branch does. An exclusion recorded against any ID the disk can hold +// (id, its current ID or its machine-scoped ID) applies, so a split survives a +// re-key; once the identity spans machines, id no longer names one disk, so a +// split recorded against it applies to every copy (erring towards an extra +// row, never a merge the operator forbade). Manual links recorded against id +// stop applying for the same reason. When every disk carrying the identity +// sits on another machine, each +// copy takes the identity keyed to the machine of its canonical parent at that +// point, and the copy holding id is re-keyed too. So machines can be ingested +// in any order, and id never passes between disks that coexist on different +// machines. Like any serial-keyed ID it still passes to a same-serial disk +// that appears once the previous holder is gone, and a disk's ID changes when +// a same-serial disk first appears on another machine and back when it goes +// away. More than one joinable disk joins none, except that the holder of id +// keeps it. +func (rr *ResourceRegistry) physicalDiskIDForMachineLocked(id, candidateID string, incoming *Resource, source DataSource, onlyMissing bool) string { + key := physicalDiskHardwareKey(incoming.Identity) + if key == "" { + return id + } + siblings := rr.physicalDisksWithHardwareKeyLocked(key) + if len(siblings) == 0 { + return id + } + var joinable []*Resource + excluded := false + for _, sibling := range siblings { + if rr.physicalDiskMachinesConflictLocked(sibling, incoming, source) { + continue + } + if rr.isExcluded(sibling.ID, candidateID) || rr.isExcluded(id, candidateID) || + rr.isExcluded(rr.physicalDiskScopedIDLocked(sibling), candidateID) { + excluded = true + continue + } + joinable = append(joinable, sibling) + } + if len(joinable) == 1 { + return joinable[0].ID + } + for _, sibling := range joinable { + if sibling.ID == id { + return id + } + } + if excluded && len(joinable) == 0 { + return rr.physicalDiskFallbackIDLocked(candidateID, incoming, source) + } + scopedID := rr.physicalDiskScopedIDLocked(incoming) + if scopedID == "" { + return id + } + if holder := rr.resources[scopedID]; holder != nil && + (holder.Type != ResourceTypePhysicalDisk || rr.physicalDiskMachinesConflictLocked(holder, incoming, source)) { + return rr.physicalDiskFallbackIDLocked(candidateID, incoming, source) + } + if len(joinable) == 0 && !onlyMissing { + for _, sibling := range siblings { + if sibling.ID == id { + rr.rekeyPhysicalDiskLocked(sibling, rr.physicalDiskScopedIDLocked(sibling)) + } + } + } + return scopedID +} + +// physicalDiskMachinesConflictLocked reports whether two disks carrying one +// hardware identity provably sit on different machines. One reporter placing +// the identity on two machines is proof even when their hostnames match, as +// cloned VMs keep both. Different reporters may sit on machine resources the +// registry never merged, such as a TrueNAS system and the agent running on it, +// so there differing machines conflict only when both disks name hosts with no +// hostname in common. An unknown machine is never a conflict. +func (rr *ResourceRegistry) physicalDiskMachinesConflictLocked(existing, incoming *Resource, source DataSource) bool { + incomingMachine := rr.physicalDiskMachineLocked(incoming.ParentID) + reported, ok := existing.parentBySource[source] + if !ok && existing.ParentID != nil && len(existing.Sources) == 1 && existing.Sources[0] == source { + // Per-source parents do not survive serialization; a single-source + // disk's own parent is that source's. + reported, ok = *existing.ParentID, true + } + if ok && incomingMachine != "" { + if reportedMachine := rr.physicalDiskMachineLocked(&reported); reportedMachine != "" && reportedMachine != incomingMachine { + return true + } + } + if existingMachine := rr.physicalDiskMachineLocked(existing.ParentID); existingMachine != "" && existingMachine == incomingMachine { + return false + } + if physicalDiskHostname(existing) == "" || physicalDiskHostname(incoming) == "" { + return false + } + return !identitiesShareHostname(existing.Identity, incoming.Identity) +} + +// physicalDiskMachineLocked resolves a disk's parent to the machine it sits +// on: the nearest host-shaped ancestor (a Proxmox node, agent host or TrueNAS +// system above a pool or Unraid array), else the topmost known ancestor. +func (rr *ResourceRegistry) physicalDiskMachineLocked(parentID *string) string { + if parentID == nil { + return "" + } + id := CanonicalResourceID(strings.TrimSpace(*parentID)) + for depth := 0; id != "" && depth < 8; depth++ { + parent := rr.resources[id] + if parent == nil || parent.Type == ResourceTypeAgent || parent.ParentID == nil { + return id + } + next := CanonicalResourceID(strings.TrimSpace(*parent.ParentID)) + if next == "" || next == id { + return id + } + id = next + } + return id +} + +// physicalDiskScopedIDLocked keys a disk's hardware identity to its machine, +// or to its hostname when the machine is unknown. +func (rr *ResourceRegistry) physicalDiskScopedIDLocked(disk *Resource) string { + key := physicalDiskHardwareKey(disk.Identity) + scope := rr.physicalDiskScopeLocked(disk) + if key == "" || scope == "" { + return "" + } + return buildHashID(ResourceTypePhysicalDisk, key+"@"+scope) +} + +// physicalDiskFallbackIDLocked returns the observation's source-specific ID, +// keyed to its machine when another machine's disk already holds it: agent +// disk source IDs are the bare serial, so one candidate ID serves every host. +func (rr *ResourceRegistry) physicalDiskFallbackIDLocked(candidateID string, incoming *Resource, source DataSource) string { + holder := rr.resources[candidateID] + if holder == nil || (holder.Type == ResourceTypePhysicalDisk && !rr.physicalDiskMachinesConflictLocked(holder, incoming, source)) { + return candidateID + } + if scope := rr.physicalDiskScopeLocked(incoming); scope != "" { + return buildHashID(ResourceTypePhysicalDisk, candidateID+"@"+scope) + } + return candidateID +} + +func (rr *ResourceRegistry) physicalDiskScopeLocked(disk *Resource) string { + if machine := rr.physicalDiskMachineLocked(disk.ParentID); machine != "" { + return "machine:" + machine + } + if hostname := physicalDiskHostname(disk); hostname != "" { + return "host:" + hostname + } + return "" +} + +// physicalDiskHardwareKey is the stable key chooseNewID hashes into a disk's +// hardware-keyed canonical ID, or "" when the disk has no hardware identity. +func physicalDiskHardwareKey(identity ResourceIdentity) string { + if machineID := strings.TrimSpace(identity.MachineID); machineID != "" { + return "machine:" + machineID + } + if dmiUUID := strings.TrimSpace(identity.DMIUUID); dmiUUID != "" { + return "dmi:" + dmiUUID + } + return "" +} + +// physicalDiskHostname is the disk's least full hostname, so a scoped ID does +// not depend on the order merges appended hostnames in. +func physicalDiskHostname(disk *Resource) string { + least := "" + for _, hostname := range disk.Identity.Hostnames { + if NormalizeHostname(hostname) == "" { + continue + } + if full := NormalizeFullHostname(hostname); least == "" || full < least { + least = full + } + } + return least +} + +func (rr *ResourceRegistry) indexPhysicalDiskHardwareLocked(disk *Resource) { + if disk == nil || disk.Type != ResourceTypePhysicalDisk { + return + } + key := physicalDiskHardwareKey(disk.Identity) + if key == "" { + return + } + if rr.physicalDisksByHardware == nil { + rr.physicalDisksByHardware = make(map[string]map[string]struct{}) + } + ids := rr.physicalDisksByHardware[key] + if ids == nil { + ids = make(map[string]struct{}) + rr.physicalDisksByHardware[key] = ids + } + ids[disk.ID] = struct{}{} +} + +// physicalDisksWithHardwareKeyLocked returns the live disks carrying key, +// sorted by ID, and drops index entries that no longer match. +func (rr *ResourceRegistry) physicalDisksWithHardwareKeyLocked(key string) []*Resource { + ids := rr.physicalDisksByHardware[key] + disks := make([]*Resource, 0, len(ids)) + for id := range ids { + disk := rr.resources[id] + if disk == nil || disk.ID != id || disk.Type != ResourceTypePhysicalDisk || physicalDiskHardwareKey(disk.Identity) != key { + delete(ids, id) + continue + } + disks = append(disks, disk) + } + sort.Slice(disks, func(i, j int) bool { return disks[i].ID < disks[j].ID }) + return disks +} + +// rekeyPhysicalDiskLocked moves a disk to newID, carrying its source mappings, +// identity index entries, retired-ID claims, child parent references and +// relationship endpoints. +func (rr *ResourceRegistry) rekeyPhysicalDiskLocked(disk *Resource, newID string) { + oldID := disk.ID + if newID == "" || newID == oldID || rr.resources[newID] != nil { + return + } + delete(rr.resources, oldID) + disk.ID = newID + rr.resources[newID] = disk + rr.updateSourceMappings(oldID, newID) + rr.matcher.Add(newID, disk.Identity) + rr.indexPhysicalDiskHardwareLocked(disk) + for retiredID, claimant := range rr.supersededIndex { + if claimant == oldID { + rr.supersededIndex[retiredID] = newID + } + } + for _, resource := range rr.resources { + if resource == nil { + continue + } + for index := range resource.Relationships { + relationship := &resource.Relationships[index] + if relationship.SourceID == oldID { + relationship.SourceID = newID + } + if relationship.TargetID == oldID { + relationship.TargetID = newID + } + } + } +} + func (rr *ResourceRegistry) sourceResourceID(source DataSource, sourceID string) string { rr.mu.RLock() defer rr.mu.RUnlock() diff --git a/internal/unifiedresources/registry_merge_policy_test.go b/internal/unifiedresources/registry_merge_policy_test.go index 5b7a92adb6..89fafc70b0 100644 --- a/internal/unifiedresources/registry_merge_policy_test.go +++ b/internal/unifiedresources/registry_merge_policy_test.go @@ -2,6 +2,7 @@ package unifiedresources import ( "encoding/json" + "slices" "strconv" "strings" "testing" @@ -1926,3 +1927,464 @@ func TestPhysicalDiskMergeTemperatureReadingsAcrossIngestBoundaries(t *testing.T } }) } + +const sharedDiskSerial = "SHARED-SERIAL-1" + +// sharedSerialDiskSnapshot puts one /dev/sda carrying sharedDiskSerial under +// each named PVE node of one instance, optionally with a linked host agent +// reporting that disk over SMART on every node. pve1 also carries a disk whose +// serial no other machine reports. +func sharedSerialDiskSnapshot(now time.Time, withAgents bool, nodeNames ...string) models.StateSnapshot { + var snapshot models.StateSnapshot + for _, name := range nodeNames { + node := models.Node{ + ID: proxmoxNodeSourceID("pve", name), + Name: name, + Instance: "pve", + Host: "https://" + name + ":8006", + Status: "online", + LastSeen: now, + } + if withAgents { + node.LinkedAgentID = "host-" + name + snapshot.Hosts = append(snapshot.Hosts, models.Host{ + ID: "host-" + name, + Hostname: name, + MachineID: "machine-" + name, + LinkedNodeID: node.ID, + Status: "online", + LastSeen: now, + Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{{ + Device: "/dev/sda", + Model: "SEAGATE ST4000NM0023", + Serial: sharedDiskSerial, + Type: "sas", + Temperature: 31, + Health: "PASSED", + }}}, + }) + } + snapshot.Nodes = append(snapshot.Nodes, node) + snapshot.PhysicalDisks = append(snapshot.PhysicalDisks, models.PhysicalDisk{ + ID: ProxmoxPhysicalDiskSourceID("pve", name, "/dev/sda", "", ""), + Instance: "pve", + Node: name, + DevPath: "/dev/sda", + Model: "SEAGATE ST4000NM0023", + Serial: sharedDiskSerial, + Type: "sas", + Health: "PASSED", + LastChecked: now, + }) + if name == "pve1" { + snapshot.PhysicalDisks = append(snapshot.PhysicalDisks, models.PhysicalDisk{ + ID: ProxmoxPhysicalDiskSourceID("pve", name, "/dev/sdb", "", ""), + Instance: "pve", + Node: name, + DevPath: "/dev/sdb", + Model: "Samsung SSD 870", + Serial: "UNIQUE-SERIAL-1", + Type: "ssd", + Health: "PASSED", + LastChecked: now, + }) + } + } + return snapshot +} + +// reversedSnapshot reverses every slice sharedSerialDiskSnapshot fills, so a +// test can prove IDs do not depend on ingest order. +func reversedSnapshot(snapshot models.StateSnapshot) models.StateSnapshot { + slices.Reverse(snapshot.Nodes) + slices.Reverse(snapshot.Hosts) + slices.Reverse(snapshot.PhysicalDisks) + return snapshot +} + +// sharedSerialDisksByNode indexes the registry's sharedDiskSerial disks by +// Proxmox node, failing when two claim one node or one lost its node. +func sharedSerialDisksByNode(t *testing.T, rr *ResourceRegistry) map[string]Resource { + t.Helper() + byNode := make(map[string]Resource) + for _, disk := range rr.ListByType(ResourceTypePhysicalDisk) { + if disk.PhysicalDisk == nil || disk.PhysicalDisk.Serial != sharedDiskSerial { + continue + } + if disk.Proxmox == nil || disk.Proxmox.NodeName == "" { + t.Fatalf("disk %s has no Proxmox node: %+v", disk.ID, disk.Proxmox) + } + if _, dup := byNode[disk.Proxmox.NodeName]; dup { + t.Fatalf("two disks claim node %s", disk.Proxmox.NodeName) + } + byNode[disk.Proxmox.NodeName] = disk + } + return byNode +} + +func TestPhysicalDisksSharingASerialStayOnEachMachine(t *testing.T) { + now := time.Date(2026, 10, 7, 12, 0, 0, 0, time.UTC) + for _, tc := range []struct { + name string + withAgents bool + sources []DataSource + }{ + {name: "proxmox only", sources: []DataSource{SourceProxmox}}, + {name: "linked host agents", withAgents: true, sources: []DataSource{SourceProxmox, SourceAgent}}, + } { + t.Run(tc.name, func(t *testing.T) { + rr := NewRegistry(nil) + rr.IngestSnapshot(sharedSerialDiskSnapshot(now, tc.withAgents, "pve1", "pve2", "pve3")) + + byNode := sharedSerialDisksByNode(t, rr) + if len(byNode) != 3 { + t.Fatalf("shared-serial disks by node = %d, want one per node", len(byNode)) + } + bareID := rr.canonicalIDFromIdentity(ResourceTypePhysicalDisk, ResourceIdentity{MachineID: sharedDiskSerial}) + for _, name := range []string{"pve1", "pve2", "pve3"} { + disk := byNode[name] + nodeID := rr.bySource[SourceProxmox][proxmoxNodeSourceID("pve", name)] + if disk.ParentID == nil || *disk.ParentID != nodeID { + t.Fatalf("disk on %s parent = %v, want its node %s", name, disk.ParentID, nodeID) + } + for _, source := range tc.sources { + if !containsDataSource(disk.Sources, source) { + t.Fatalf("disk on %s sources = %v, want %s merged in", name, disk.Sources, source) + } + } + if !slices.ContainsFunc(rr.GetChildren(nodeID), func(child Resource) bool { return child.ID == disk.ID }) { + t.Fatalf("node %s does not list its disk %s as a child", name, disk.ID) + } + // No machine keeps the unscoped ID, so it can never pass + // from one machine's disk to another's. + if disk.ID == bareID { + t.Fatalf("disk on %s holds the unscoped ID %s shared by every machine", name, bareID) + } + } + + uniqueID := rr.canonicalIDFromIdentity(ResourceTypePhysicalDisk, ResourceIdentity{MachineID: "UNIQUE-SERIAL-1"}) + if unique, ok := rr.Get(uniqueID); !ok || unique.PhysicalDisk == nil || unique.PhysicalDisk.Serial != "UNIQUE-SERIAL-1" { + t.Fatalf("a disk only one machine reports must keep its unscoped ID %s", uniqueID) + } + + reordered := NewRegistry(nil) + reordered.IngestSnapshot(reversedSnapshot(sharedSerialDiskSnapshot(now, tc.withAgents, "pve1", "pve2", "pve3"))) + for name, disk := range sharedSerialDisksByNode(t, reordered) { + if disk.ID != byNode[name].ID { + t.Fatalf("reversed ingest moved the disk on %s from %s to %s", name, byNode[name].ID, disk.ID) + } + } + }) + } +} + +func TestPhysicalDiskIDsHoldAcrossRehydrationWhenAMachineJoins(t *testing.T) { + now := time.Date(2026, 10, 7, 12, 0, 0, 0, time.UTC) + first := NewRegistry(nil) + first.IngestSnapshot(sharedSerialDiskSnapshot(now, false, "pve2", "pve3")) + before := sharedSerialDisksByNode(t, first) + + rehydrated := NewRegistry(nil) + rehydrated.IngestResources(first.List()) + rehydrated.IngestSnapshot(sharedSerialDiskSnapshot(now.Add(time.Minute), false, "pve1", "pve2", "pve3")) + after := sharedSerialDisksByNode(t, rehydrated) + if len(after) != 3 { + t.Fatalf("shared-serial disks by node = %d, want one per node", len(after)) + } + for name, disk := range before { + if after[name].ID != disk.ID { + t.Fatalf("disk on %s moved from %s to %s when pve1 joined", name, disk.ID, after[name].ID) + } + } + + fresh := NewRegistry(nil) + fresh.IngestSnapshot(sharedSerialDiskSnapshot(now.Add(time.Minute), false, "pve1", "pve2", "pve3")) + for name, disk := range sharedSerialDisksByNode(t, fresh) { + if after[name].ID != disk.ID { + t.Fatalf("disk on %s is %s after rehydration but %s after a fresh rebuild", name, after[name].ID, disk.ID) + } + } +} + +// trueNASDiskRecords describes one TrueNAS system holding one disk, under a +// pool when pool is set. +func trueNASDiskRecords(now time.Time, system, hostname, pool, serial string) []IngestRecord { + records := []IngestRecord{{ + SourceID: "system:" + system, + Resource: Resource{Type: ResourceTypeAgent, Name: hostname, Status: StatusOnline, LastSeen: now, TrueNAS: &TrueNASData{Hostname: hostname}}, + Identity: ResourceIdentity{Hostnames: []string{hostname}}, + }} + diskParent := "system:" + system + if pool != "" { + diskParent = "pool:" + system + ":" + pool + records = append(records, IngestRecord{ + SourceID: diskParent, + ParentSourceID: "system:" + system, + Resource: Resource{Type: ResourceTypeStorage, Name: pool, Status: StatusOnline, LastSeen: now}, + Identity: ResourceIdentity{Hostnames: []string{hostname}}, + }) + } + return append(records, IngestRecord{ + SourceID: "disk:" + system + ":sda", + ParentSourceID: diskParent, + Resource: Resource{ + Type: ResourceTypePhysicalDisk, Name: "sda", Status: StatusOnline, LastSeen: now, + PhysicalDisk: &PhysicalDiskMeta{DevPath: "/dev/sda", Serial: serial, Wearout: WearoutUnreported}, + }, + Identity: ResourceIdentity{MachineID: serial, Hostnames: []string{hostname}}, + }) +} + +func TestPhysicalDiskReportersOnOneMachineJoinAcrossGroupingParents(t *testing.T) { + now := time.Date(2026, 10, 7, 12, 0, 0, 0, time.UTC) + agentOnArchive := models.StateSnapshot{Hosts: []models.Host{{ + ID: "host-archive", Hostname: "archive", MachineID: "machine-archive", Status: "online", LastSeen: now, + Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{{ + Device: "/dev/sda", Model: "WDC WD80EFAX", Serial: sharedDiskSerial, Type: "sata", Temperature: 30, Health: "PASSED", + }}}, + }}} + + // The TrueNAS API and the agent on the same box report one disk under + // different parents (pool and agent host) that the registry never merged. + rr := NewRegistry(nil) + rr.IngestSnapshot(agentOnArchive) + rr.IngestRecords(SourceTrueNAS, trueNASDiskRecords(now, "a", "archive", "tank", sharedDiskSerial)) + disks := rr.ListByType(ResourceTypePhysicalDisk) + if len(disks) != 1 || !containsDataSource(disks[0].Sources, SourceAgent) || !containsDataSource(disks[0].Sources, SourceTrueNAS) { + t.Fatalf("disks = %+v, want the agent and TrueNAS rows of one box merged", disks) + } + + // A cloned TrueNAS keeps both the serial and the hostname; one reporter + // placing the serial on two systems still proves two disks. + rr.IngestRecords(SourceTrueNAS, trueNASDiskRecords(now, "b", "archive", "tank", sharedDiskSerial)) + disks = rr.ListByType(ResourceTypePhysicalDisk) + if len(disks) != 2 { + t.Fatalf("disks = %d, want the cloned system's disk kept apart", len(disks)) + } + bySystem := make(map[string]Resource) + for _, disk := range disks { + for _, source := range []string{"disk:a:sda", "disk:b:sda"} { + if rr.bySource[SourceTrueNAS][source] == disk.ID { + bySystem[source] = disk + } + } + } + if len(bySystem) != 2 { + t.Fatalf("TrueNAS disk mappings = %+v, want one disk per system", rr.bySource[SourceTrueNAS]) + } + if !containsDataSource(bySystem["disk:a:sda"].Sources, SourceAgent) { + t.Fatalf("system a disk sources = %v, want the agent row still merged", bySystem["disk:a:sda"].Sources) + } + + // A disk moving into a pool on the same machine is the same disk. + moved := NewRegistry(nil) + moved.IngestRecords(SourceTrueNAS, trueNASDiskRecords(now, "a", "archive", "", sharedDiskSerial)) + moved.IngestRecords(SourceTrueNAS, trueNASDiskRecords(now.Add(time.Minute), "a", "archive", "tank", sharedDiskSerial)) + if disks := moved.ListByType(ResourceTypePhysicalDisk); len(disks) != 1 { + t.Fatalf("disks after moving into a pool = %d, want 1", len(disks)) + } +} + +func TestPhysicalDiskWithoutItsNodeRowJoinsItsOwnMachinesDisk(t *testing.T) { + now := time.Date(2026, 10, 7, 12, 0, 0, 0, time.UTC) + snapshot := sharedSerialDiskSnapshot(now, false, "pve1", "pve2") + // pve3 has an agent but no node row this poll, so its PVE disk arrives + // without a parent while the serial already spans other machines. + snapshot.Hosts = append(snapshot.Hosts, models.Host{ + ID: "host-pve3", Hostname: "pve3", MachineID: "machine-pve3", Status: "online", LastSeen: now, + Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{{ + Device: "/dev/sda", Model: "SEAGATE ST4000NM0023", Serial: sharedDiskSerial, Type: "sas", Temperature: 31, Health: "PASSED", + }}}, + }) + snapshot.PhysicalDisks = append(snapshot.PhysicalDisks, models.PhysicalDisk{ + ID: ProxmoxPhysicalDiskSourceID("pve", "pve3", "/dev/sda", "", ""), Instance: "pve", Node: "pve3", + DevPath: "/dev/sda", Model: "SEAGATE ST4000NM0023", Serial: sharedDiskSerial, Type: "sas", Health: "PASSED", LastChecked: now, + }) + + rr := NewRegistry(nil) + rr.IngestSnapshot(snapshot) + byNode := sharedSerialDisksByNode(t, rr) + if len(byNode) != 3 { + t.Fatalf("shared-serial disks by node = %d, want one per node", len(byNode)) + } + if sources := byNode["pve3"].Sources; !containsDataSource(sources, SourceAgent) || !containsDataSource(sources, SourceProxmox) { + t.Fatalf("pve3 disk sources = %v, want its agent and parentless PVE rows merged", sources) + } +} + +func TestPhysicalDiskSiblingJoinHonoursManualExclusions(t *testing.T) { + now := time.Date(2026, 10, 7, 12, 0, 0, 0, time.UTC) + // An agent on pve1 that the registry never linked to the node reports the + // disk after both nodes' rows; hostname evidence alone joins it to pve1's. + agentOnPVE1 := models.StateSnapshot{Hosts: []models.Host{{ + ID: "host-pve1", Hostname: "pve1", MachineID: "machine-pve1", Status: "online", LastSeen: now, + Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{{ + Device: "/dev/sda", Model: "SEAGATE ST4000NM0023", Serial: sharedDiskSerial, Type: "sas", Temperature: 31, Health: "PASSED", + }}}, + }}} + ingest := func(store ResourceStore) *ResourceRegistry { + rr := NewRegistry(store) + rr.IngestSnapshot(sharedSerialDiskSnapshot(now, false, "pve1", "pve2")) + rr.IngestSnapshot(agentOnPVE1) + return rr + } + + joined := ingest(nil) + pve1Disk := sharedSerialDisksByNode(t, joined)["pve1"] + if !containsDataSource(pve1Disk.Sources, SourceAgent) { + t.Fatalf("pve1 disk sources = %v, want the agent row joined", pve1Disk.Sources) + } + + store := NewMemoryStore() + agentCandidate := buildHashID(ResourceTypePhysicalDisk, string(SourceAgent)+":"+normalizeSourceID(sharedDiskSerial)) + if err := store.AddExclusion(ResourceExclusion{ResourceA: pve1Disk.ID, ResourceB: agentCandidate}); err != nil { + t.Fatal(err) + } + split := ingest(store) + if disk, ok := split.Get(pve1Disk.ID); !ok || containsDataSource(disk.Sources, SourceAgent) { + t.Fatalf("excluded pve1 disk = %+v, want it kept apart from the agent row", disk) + } + if agentDisk, ok := split.Get(agentCandidate); !ok || !containsDataSource(agentDisk.Sources, SourceAgent) { + t.Fatalf("agent row should keep its source-specific ID %s once excluded", agentCandidate) + } +} + +func TestPhysicalDiskClonesStayApartAfterSerializedRehydration(t *testing.T) { + now := time.Date(2026, 10, 7, 12, 0, 0, 0, time.UTC) + first := NewRegistry(nil) + first.IngestRecords(SourceTrueNAS, trueNASDiskRecords(now, "a", "archive", "tank", sharedDiskSerial)) + payload, err := json.Marshal(first.List()) + if err != nil { + t.Fatal(err) + } + var persisted []Resource + if err := json.Unmarshal(payload, &persisted); err != nil { + t.Fatal(err) + } + + rehydrated := NewRegistry(nil) + rehydrated.IngestResources(persisted) + // A clone keeps the hostname; the persisted disk lost its per-source + // parents, so only its own parent says which system TrueNAS saw it on. + rehydrated.IngestRecords(SourceTrueNAS, trueNASDiskRecords(now, "b", "archive", "tank", sharedDiskSerial)) + if disks := rehydrated.ListByType(ResourceTypePhysicalDisk); len(disks) != 2 { + t.Fatalf("disks = %d, want the clone's disk kept apart after rehydration", len(disks)) + } +} + +func TestPhysicalDiskRekeyCarriesRetiredIDClaims(t *testing.T) { + now := time.Date(2026, 10, 7, 12, 0, 0, 0, time.UTC) + records := trueNASDiskRecords(now, "a", "archive-a", "tank", sharedDiskSerial) + records[len(records)-1].SupersededCanonicalIDs = []string{"physical_disk-retired"} + rr := NewRegistry(nil) + rr.IngestRecords(SourceTrueNAS, records) + bareID := rr.canonicalIDFromIdentity(ResourceTypePhysicalDisk, ResourceIdentity{MachineID: sharedDiskSerial}) + if got := rr.supersededResourceIDLocked("physical_disk-retired"); got != bareID { + t.Fatalf("retired ID resolves to %q, want the disk %s", got, bareID) + } + + rr.IngestRecords(SourceTrueNAS, trueNASDiskRecords(now, "b", "archive-b", "tank", sharedDiskSerial)) + systemADisk := rr.bySource[SourceTrueNAS]["disk:a:sda"] + if systemADisk == bareID || systemADisk == "" { + t.Fatalf("system a disk = %q, want it re-keyed off the unscoped ID", systemADisk) + } + if got := rr.supersededResourceIDLocked("physical_disk-retired"); got != systemADisk { + t.Fatalf("retired ID resolves to %q after the re-key, want %s", got, systemADisk) + } +} + +func TestPhysicalDiskSplitSurvivesARekey(t *testing.T) { + now := time.Date(2026, 10, 7, 12, 0, 0, 0, time.UTC) + agentDisk := func(id, hostname string) models.Host { + return models.Host{ + ID: id, Hostname: hostname, MachineID: "machine-" + hostname, Status: "online", LastSeen: now, + Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{{ + Device: "/dev/sda", Model: "WDC WD80EFAX", Serial: sharedDiskSerial, Type: "sata", Temperature: 30, Health: "PASSED", + }}}, + } + } + trueNASCandidate := buildHashID(ResourceTypePhysicalDisk, string(SourceTrueNAS)+":"+normalizeSourceID("disk:a:sda")) + bareID := MachineIdentityCanonicalID(ResourceTypePhysicalDisk, sharedDiskSerial) + ingest := func(t *testing.T, excludedID string, hosts ...models.Host) *ResourceRegistry { + t.Helper() + store := NewMemoryStore() + if err := store.AddExclusion(ResourceExclusion{ResourceA: excludedID, ResourceB: trueNASCandidate}); err != nil { + t.Fatal(err) + } + rr := NewRegistry(store) + rr.IngestSnapshot(models.StateSnapshot{Hosts: hosts}) + rr.IngestRecords(SourceTrueNAS, trueNASDiskRecords(now, "a", "archive", "tank", sharedDiskSerial)) + return rr + } + split := func(t *testing.T, rr *ResourceRegistry) { + t.Helper() + trueNASDisk, ok := rr.Get(trueNASCandidate) + if !ok || containsDataSource(trueNASDisk.Sources, SourceAgent) { + t.Fatalf("TrueNAS row = %+v, want it kept on %s apart from the agent row", trueNASDisk, trueNASCandidate) + } + } + + t.Run("split recorded against the unscoped ID after another machine re-keyed the disk", func(t *testing.T) { + rr := ingest(t, bareID, agentDisk("host-archive", "archive"), agentDisk("host-other", "other")) + if _, ok := rr.Get(bareID); ok { + t.Fatalf("the unscoped ID %s should be re-keyed once two machines report the serial", bareID) + } + split(t, rr) + }) + t.Run("split recorded against the scoped ID before the serial spans machines", func(t *testing.T) { + scoped := ingest(t, bareID, agentDisk("host-archive", "archive"), agentDisk("host-other", "other")) + archiveAgentDisk := "" + for _, disk := range scoped.ListByType(ResourceTypePhysicalDisk) { + if disk.ParentID != nil && *disk.ParentID == scoped.bySource[SourceAgent]["host-archive"] { + archiveAgentDisk = disk.ID + } + } + if archiveAgentDisk == "" || archiveAgentDisk == bareID { + t.Fatalf("archive agent disk = %q, want a machine-scoped ID", archiveAgentDisk) + } + split(t, ingest(t, archiveAgentDisk, agentDisk("host-archive", "archive"))) + }) +} + +func TestPhysicalDiskExclusionFallbackStaysPerMachine(t *testing.T) { + now := time.Date(2026, 10, 7, 12, 0, 0, 0, time.UTC) + agents := models.StateSnapshot{} + for _, name := range []string{"pve1", "pve2"} { + agents.Hosts = append(agents.Hosts, models.Host{ + ID: "host-" + name, Hostname: name, MachineID: "machine-" + name, Status: "online", LastSeen: now, + Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{{ + Device: "/dev/sda", Model: "SEAGATE ST4000NM0023", Serial: sharedDiskSerial, Type: "sas", Temperature: 31, Health: "PASSED", + }}}, + }) + } + probe := NewRegistry(nil) + probe.IngestSnapshot(sharedSerialDiskSnapshot(now, false, "pve1", "pve2")) + byNode := sharedSerialDisksByNode(t, probe) + + // Both nodes' disks are split from the agent observation, whose + // source-specific ID is the same on every host. + store := NewMemoryStore() + agentCandidate := buildHashID(ResourceTypePhysicalDisk, string(SourceAgent)+":"+normalizeSourceID(sharedDiskSerial)) + for _, name := range []string{"pve1", "pve2"} { + if err := store.AddExclusion(ResourceExclusion{ResourceA: byNode[name].ID, ResourceB: agentCandidate}); err != nil { + t.Fatal(err) + } + } + rr := NewRegistry(store) + rr.IngestSnapshot(sharedSerialDiskSnapshot(now, false, "pve1", "pve2")) + rr.IngestSnapshot(agents) + + agentDisks := 0 + for _, disk := range rr.ListByType(ResourceTypePhysicalDisk) { + if !containsDataSource(disk.Sources, SourceAgent) { + continue + } + agentDisks++ + if containsDataSource(disk.Sources, SourceProxmox) { + t.Fatalf("disk %s merged an excluded agent row into a node's disk", disk.ID) + } + } + if agentDisks != 2 { + t.Fatalf("agent disks = %d, want one per host rather than both hosts on the shared fallback ID", agentDisks) + } +} From 8526011cbd89a1bbcdc2f0203ddfa7675e16d073 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 13:04:54 +0100 Subject: [PATCH 10/12] Take main's wall-clock wording for the relative-age rule Main replaced the note about the relative-time clock trailing the wall clock: every read now returns the wall clock. Adopt that paragraph as it stands so the merge with main is clean; the read-time exception is restated after it once main is merged. --- .../subsystems/frontend-primitives.md | 200 ++++++++++++++---- 1 file changed, 155 insertions(+), 45 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/frontend-primitives.md b/docs/release-control/v6/internal/subsystems/frontend-primitives.md index 19606688a1..bf1638080e 100644 --- a/docs/release-control/v6/internal/subsystems/frontend-primitives.md +++ b/docs/release-control/v6/internal/subsystems/frontend-primitives.md @@ -22,6 +22,27 @@ All drawer History fallbacks must distinguish current observations from retained, unavailable or freshness-unknown guest readings, using original memory evidence and filesystem read reasons. +### Drawer attention detail line and temperature alert floor — issue #2068 + +`DrawerAttentionSection` items accept an optional `detail` (a muted second +line) and `title` (hover text). `TemperatureGauge` accepts `alertSeverity` +and `title`, and `getTemperatureTextClass` treats an open alert's severity as +a floor: a reading that has dipped under an open alert's trigger keeps the +alert's tone, while the reading still wins when it is worse. + +### Alert card and incident panel format held alerts through one helper — issue #2068 + +`AlertOverviewAlertCard` and `AlertResourceIncidentsPanel` describe an open +threshold alert through `features/alerts/metricAlertPresentation.ts` +(summary, clear rule, `alertLevel` and `clearLevel`) instead of their own unit +guesses or the breach message. `useAlertHistoryState` exposes its active-alert +accessor so the incident panel matches its open occurrence to the live alert +without a second store read, and the panel reads `useRelativeTimeNow` so a +reading that stops updating turns stale on screen. From the `sm` breakpoint the +card's left column (status icon and text) keeps a 16rem floor and the text +breaks long words, so the action buttons wrap instead of squeezing the reading +to its longest word beside them. + ### Canonical drawer History preserves guest read provenance The shared resource drawer passes selected memory observation state/source/time @@ -404,6 +425,12 @@ disk header and tooltips. Structured ZFS scan activity supplies a compact badge only during reported rebuild activity; its complete provider summary remains available. Stable disk History catalog/organisation/access ownership is unchanged. +The physical-disk verdict words and their phone forms are `Needs Attention` +(`Attention`), `Running Hot` (`Hot`) and `Replace Now` (`Replace`), all from +`getPhysicalDiskHealthCompactLabel` in `diskPresentation.ts`. `Running Hot` is +red and starts at the disk's alert trigger, from the same thresholds object +that colours its Temp cell (`getPhysicalDiskTemperatureThresholds`). + Controller expansions carry the supplied kind-specific targets, absolute times, duration and cluster/namespace when the phone row omits those columns. Proxmox coverage distinguishes independent backups from guest-local snapshots; By date @@ -499,6 +526,22 @@ SharedPrimitives.guardrails.test.ts protects this composition; component tests preserve mount counts and totals. The disk-mounts qualification fixture checks short/long lists, themes and keyboard reachability with production CSS. +### Retained disk usage + +DisksCard takes an optional `lastKnownReason` for usage figures that are +retained rather than current. The machine drawer passes the host-agent +stopped-reporting reason when `agent.stale` is set, the same signal that marks +its Thermals rows "(last known)". The card keeps every figure, because a disk +that filled before the machine went quiet is evidence. The total and each mount +read "Last known N%" with used and total bytes, in muted text titled "Last known +reading, not current: ". Threshold text colour, the aggregate +`StackedDiskBar` and the per-mount bars are dropped, as the guest drawer does +for retained filesystem rows. Mount rows may wrap, with the label capped at the +row width, so on a phone a long mountpoint takes its own line instead of +squeezing the figures out of the card. `DisksCard.test.tsx` covers both +readings and SharedPrimitives.guardrails.test.ts pins the branch and the drawer +wiring. + ### Ollama credential editing The provider panel exposes the existing Basic Auth configuration. Saved passwords are represented by presence text, never a placeholder secret or input value. @@ -666,20 +709,27 @@ existing picker unchanged. The default Workloads metric presentation keeps compact progress bars at rest. A fine-pointer preview or keyboard focus on one guest row replaces CPU, memory, -and disk together with the existing `MetricMiniSparkline` presentation without -changing row height; touch pointer entry does not trigger this transient lens, -and the persistent Trends View choice remains the touch-accessible fallback. +disk, Net I/O, and Disk I/O together with the existing `MetricMiniSparkline` +presentation without changing row height; touch pointer entry does not trigger +this transient lens, and the persistent Trends View choice remains the +touch-accessible fallback. Both I/O cells render through one rate renderer in +the lens and in Trends: the chart keeps a compact current pair beside it +(`MetricMiniSparklineRatePair`, the at-rest ↓/↑ and R/W glyphs coloured like +the series they label) while the full rates stay in the chart's accessible +label, and a stopped guest or unavailable I/O keeps its dash. The active chart owns its local tooltip while its normalized cursor position is shared across sibling charts in that guest row, so every guide represents the same relative point in the selected history range. Leaving the row clears the -cursor and restores all three bars together. The lens mounts with a short +cursor and restores the bars and I/O readouts together. The lens mounts with a short reduced-motion-safe fade and must not leave both bar and chart semantics in the accessibility tree simultaneously. -Bar mode resolves history only for that active guest through its canonical -metrics target and the selected compact range; it must not start an -estate-wide chart request merely because the range changes. The active request -key is stable across equivalent live guest snapshots, and leaving the row or -selecting another range aborts superseded browser work. Persistent Trends may +Bar mode resolves history per guest through each guest's canonical metrics +target and the selected compact range, warming only a bounded window of +mounted and adjacent rows with the active guest first; it must not start an +estate-wide chart request merely because the range changes. The request key is +stable across equivalent live guest snapshots, and selecting another range +aborts superseded browser work. Leaving a row does not cancel its read, which +settles into the bounded row cache. Persistent Trends may retain the shared estate reader, but range changes must clear prior-range data unless an exact-key cache entry exists. @@ -2898,9 +2948,9 @@ Agent`), with the plain-language source phrase available through accessible `pveVersion` or a Pulse Agent report whose OS identity resolves to Unraid or Proxmox VE. They must omit the version rather than showing unrelated collector OS versions, such as Debian 12, beside an API-backed PVE badge. - Shared row primitives that render Proxmox node identity, including - `frontend-modern/src/components/shared/NodeGroupHeader.tsx`, must route raw - PVE manager payloads through + Surfaces that render Proxmox node versions, including the Proxmox page + model at `frontend-modern/src/features/proxmox/proxmoxPageModel.ts`, must + route raw PVE manager payloads through `frontend-modern/src/utils/proxmoxVersion.ts` rather than inlining page-local parsing or falling back to unrelated agent OS versions. System title metadata must apply the same identity rule: once the primary @@ -3276,9 +3326,9 @@ default` instead of fusing provider and badge text such as Platform-first top-level pages registered through `frontend-modern/src/App.tsx` must stay chrome-only and route through the canonical app shell: each per-platform surface owns navigation and sub-tab - chrome, then embeds the canonical `WorkloadsSurface`, `StorageSurface`, - `RecoverySurface`, or `UnifiedResourceTable` in `embedded tableOnly` mode - with a forced platform or source filter. Per-platform features must not + chrome, then embeds the canonical `WorkloadsSurface`, `StorageSurface`, or + `RecoverySurface` in `embedded tableOnly` mode with a forced platform or + source filter. Per-platform features must not fork their own table primitives, header layouts, or summary cards when a shared canonical surface already exists; new shared platform-page primitives live under `frontend-modern/src/features/platformPage/` so the @@ -4382,12 +4432,14 @@ The shared table chrome now allows `TableCardHeader` to expose a right-aligned action slot, currently used by the Workloads/Proxmox metric display control. That slot belongs to the table header band and must not reintroduce nested cards or page-local toolbar wrappers inside `TableCard`. Proxmox host grouping -also extends the shared `NodeGroupHeader` row pattern: host metrics may align -with workload table columns, but the shared primitive owns the header/table -shell boundary rather than platform pages copying their own card headers. -Compact PVE version text in that header must come from the shared Proxmox -version formatter so raw `pve-manager/...` payloads and platform-page host -version cells stay consistent. +also extends the shared `NodeGroupHeader` row pattern, which owns the +header/table shell boundary rather than platform pages copying their own card +headers. `frontend-modern/src/components/shared/NodeGroupHeader.tsx` renders +node identity only (status dot, linked name, cluster and agent badges); in +table-row mode that identity sits in one cell spanning the table. It carries no +per-column cell renderer and no inline +version, temperature, or uptime facts: host metrics and versions belong to the +platform page's host table, so a group row must not duplicate them. Mobile navigation now recognizes `proxmox` as a first-class platform tab in the shared priority model so app-shell ordering remains centralized. @@ -4649,6 +4701,21 @@ Each query run receives an `AbortSignal`; changing the source, resetting the query, or unmounting its owner must abort the superseded browser request before starting replacement work. Consumers must forward that signal through their API/cache layer when the transport supports cancellation. +A poll (`pollMs`) does not replace a read that an earlier poll started while +that read is still in flight; it skips the tick. The API answers 429 with +`Retry-After: 60` and `apiFetch` honours up to two minutes, so a polled read +slower than the poll interval would otherwise be discarded on every tick and +never settle. Any other read (a refresh, a source change, a remount +revalidation) stays replaceable at the next tick, so a slow or hung refresh is +replaced at most once before polling protects its replacement. A polled read +still in flight at the first poll tick at or after `STALLED_QUERY_READ_MS` (150 +seconds) is presumed stalled: that tick replaces it and settles it as a failed +read, clearing `loading`, setting `resolvedOnce` and publishing a timed-out +`error`, so no consumer is left waiting on a read that never returns. Each +consumer renders that failure as it renders any other (the Proxmox Replication +table shows its error and Retry; workload sparklines keep retained series). A +transport that always takes longer than that deadline plus one poll interval +never publishes. The settings reporting shell now also owns a deliberate split between historical performance reports and current-state VM inventory export. `frontend-modern/src/components/Settings/ReportingPanel.tsx`, @@ -5210,20 +5277,10 @@ cells: a value formatted through `formatPlatformTableRelativeTimeValue` or and tooltips, an open drawer's summary rows and annotations) passes `now` from the owning component's `useRelativeTimeNow`, and a countdown or freshness band derived from the same time (replication Next sync, backup age bands) reads it -too. The clock can trail the wall clock by up to one tick, so a check that -treats a future time as invalid (the backup age band) measures from the later -of the two. -The rule has one deliberate exception. The age of a latest reading (last used, -last seen, last success, last checked) on data the surface reads once and does -not re-read stays the age at read time, because a moving age over a snapshot -that never refreshes claims the reading stopped when it may not have. Such a -surface either re-reads the snapshot in the background, as Proxmox replication -and the external watchdog panel do, or keeps the read-time age, as the Patrol -attention detail does for Last seen. The external watchdog panel orders its -re-reads by request, so a slow or hung read can neither pin it to an older -snapshot nor overwrite a newer outcome while its ages keep moving. An immutable event time (when a delivery -was attempted, a transition happened, a policy was set) ages correctly over any -snapshot and reads the clock. +too. Every read of the clock returns the wall clock; the 30-second tick only +tells readers to re-read, so a cell that mounts between ticks never measures +from a stale time and a timestamp from the last few seconds never reads as a +future time. Read-only metadata badges follow the same primitive-owned shell rule. `frontend-modern/src/components/shared/MetadataBadge.tsx` owns filled and outlined appearances, compact sizing, shape, typed tone vocabulary, fit @@ -5311,6 +5368,18 @@ That tooltip owner now also holds the CSP-safe hover contract: chart tooltips must render inside the chart surface with model-owned layout and SVG/attribute positioning, not through fixed portals or inline `left`/`top` style attributes that violate the public demo CSP. +The same CSP rule binds every runtime component, not only these owners. Solid +compiles a static `style` value (a `style="..."` string, or any literal entry +of a `style={{ ... }}` object) into the element's template HTML, and the +production `style-src 'self' 'nonce-...'` policy reports each one as a +`style-src-attr` violation when that template is parsed. Static styling must be +a class: a Tailwind utility, or an arbitrary property such as the +`[overflow-anchor:none]` on drawer tab panels. Only dynamic values may use +`style`, because Solid applies those through CSSOM. +`frontend-modern/src/components/shared/SharedPrimitives.guardrails.test.ts` +compiles every runtime `.tsx` that mentions `style` with the client-build Solid +preset and fails on any `style` attribute or `