From 1c101b024d7adb43cbcb615272b0558b1352fd3e Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Tue, 6 Oct 2026 23:26:46 +0100 Subject: [PATCH] Replace stored alerts with each payload instead of merging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The websocket store wrote every incoming active alert with setActiveAlerts(id, alert). Solid's path setter shallow-merges into an existing object, so a field the server stops sending kept its old value in the browser. After a backend restart, threshold alerts are restored without metricStatus until their next evaluation, but an open tab kept the pre-restart live reading and the node drawer and node table went on saying "78°C now" for a reading the server no longer reports. An unacknowledge from another session left ackTime and ackUser behind the same way, as did a dropped nodeDisplayName or metadata. Active and recently resolved alerts are now written with reconcile, so omitted keys are removed on the snapshot, keyed-delta and REST recovery commits alike. reconcile diffs nested values in place, so an unchanged alert's live status no longer re-notifies its readers on every frame, which the shallow merge did by swapping in a fresh object each time. The pending acknowledgement hold is unchanged: it still skips payloads that contradict an unconfirmed local acknowledge or unacknowledge. updateAlert now stores a copy of each local update. Because payloads reconcile the stored alert in place, a snooze's optimistic record, which shares nested objects with the record the caller keeps for rollback, would otherwise have that rollback record rewritten by the next payload. --- .../v6/internal/subsystems/alerts.md | 12 + .../subsystems/performance-and-scalability.md | 8 +- .../3b5ee6cfe34d3e8c.json | 40 +++ .../__tests__/websocket-unified.test.ts | 321 +++++++++++++++++- frontend-modern/src/stores/websocket.ts | 20 +- 5 files changed, 395 insertions(+), 6 deletions(-) create mode 100644 frontend-modern/browser-verification/3b5ee6cfe34d3e8c.json diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index c80daddd90..c7e098c5ac 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -787,6 +787,18 @@ that fast path entirely: active-alert and resolved-alert stores keep their own commit path, resource `alerts` facets are not in the fast-path allow-list, and a row whose patch touches alert-relevant structure always takes the full canonical merge. +That commit path replaces each stored alert with the server's payload for its +id instead of merging into it. A field the server omits is removed from the +store, whether the payload came from a socket snapshot, a keyed delta, or REST +recovery: a threshold alert restored after a restart carries no `metricStatus` +until its next evaluation, so the previous live reading must not keep reading +as "now"; an unacknowledge from another session clears `ackTime` and +`ackUser`; a dropped `nodeDisplayName` or `metadata` disappears. Resolved +alerts follow the same rule. The pending acknowledgement hold still skips a +payload that contradicts an unconfirmed local acknowledge or unacknowledge, +and the confirming payload then replaces the optimistic copy. Because a +replacement reconciles the stored alert in place, local optimistic updates are +stored as copies, so it never rewrites a record a caller keeps for rollback. Operational evidence and lifecycle identity are typed through `internal/operationaltrust`. Evidence envelopes distinguish completeness, confidence, permissions, freshness, correlation, and bounded provider detail. diff --git a/docs/release-control/v6/internal/subsystems/performance-and-scalability.md b/docs/release-control/v6/internal/subsystems/performance-and-scalability.md index c7ea65f894..693d316073 100644 --- a/docs/release-control/v6/internal/subsystems/performance-and-scalability.md +++ b/docs/release-control/v6/internal/subsystems/performance-and-scalability.md @@ -673,7 +673,13 @@ the store with reference-stable untouched items so the per-tick deep walk of the whole projection is gone. Active alerts travel the same keyed transport (`activeAlertsDelta`, replacing the ~37KB whole-array re-ship whenever any alert changed) while their application stays immediate per the alerts -subsystem boundary. These projection baselines are socket-owned: an oversized +subsystem boundary. Each committed alert is reconciled in place against its +copy in the keyed active-alert store, so a reader of an unchanged alert there, +or of an unchanged nested value such as its live `metricStatus` or `metadata`, +is not notified although every commit carries freshly parsed objects; omitted +fields are deleted rather than shallow-merged. The `state.activeAlerts` array +is still replaced on every alert commit. These projection baselines are +socket-owned: an oversized frame invalidates resources, connected infrastructure, and active alerts together, while REST recovery hydrates display state only. A later keyed delta without a socket baseline requests the throttled recovery path instead of diff --git a/frontend-modern/browser-verification/3b5ee6cfe34d3e8c.json b/frontend-modern/browser-verification/3b5ee6cfe34d3e8c.json new file mode 100644 index 0000000000..ffc8d4beb4 --- /dev/null +++ b/frontend-modern/browser-verification/3b5ee6cfe34d3e8c.json @@ -0,0 +1,40 @@ +{ + "version": 1, + "base_sha": "8424fd65057af75e1ea3c1fc9fe29528d7c0d68e", + "verified_at": "2026-10-06T21:24:22Z", + "result": "passed", + "changed_paths": [ + "frontend-modern/src/stores/websocket.ts" + ], + "content_sha256": { + "frontend-modern/src/stores/websocket.ts": "1be5c0f23d3dbaf85a32f7b8b3906c2cb25b20bb02412c31c200ffbad9c6c2d5" + }, + "routes": [ + "/proxmox/overview", + "/alerts/overview" + ], + "viewports": [ + { + "width": 1280, + "height": 900 + }, + { + "width": 375, + "height": 812 + } + ], + "states": [ + "Private mock stack (backend + Vite from this tree) with a local-only overlay pinning node standalone2's evaluated CPU temperature and letting mock mode restore persisted alerts across restarts", + "standalone2 temperature alert latched below trigger (breach 95, live 78): node drawer Needs attention reads 'Temperature 78\u00b0C now, back under the 80\u00b0C alert level' and the node table temperature cell title carries the same summary", + "Backend restarted with the reading missing: /api/alerts/active returns the restored alert without metricStatus; the open tab reconnects and the drawer and cell title fall back to 'Node temperature at 95.0\u00b0C' within seconds", + "Reading returns: the next keyed delta adds metricStatus and the drawer reads '78\u00b0C now' again", + "Control on the unchanged store, same run: after the restart the open tab kept 'Temperature 78\u00b0C now' in the drawer and cell title for the full two-minute window", + "Alerts overview card snoozed ('Snoozed until ...', Resume shown), resumed, and acknowledged from the UI with no page errors" + ], + "interactions": [ + "Expanded the standalone2 row details in the Proxmox nodes table (desktop) and after Show all nodes (narrow)", + "Restarted the private backend while the tab stayed open, then changed the pinned reading back to 78", + "Clicked Snooze and the first preset, then Resume, then Acknowledge on the alert card", + "Acknowledged and unacknowledged through the API from another session with the tab open (card left and returned within 2s)" + ] +} diff --git a/frontend-modern/src/stores/__tests__/websocket-unified.test.ts b/frontend-modern/src/stores/__tests__/websocket-unified.test.ts index c630e3535e..2b94a4ea8c 100644 --- a/frontend-modern/src/stores/__tests__/websocket-unified.test.ts +++ b/frontend-modern/src/stores/__tests__/websocket-unified.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { createRoot } from 'solid-js'; +import { createComputed, createRoot } from 'solid-js'; // Mirrors MAX_INBOUND_WEBSOCKET_MESSAGE_BYTES in the store under test. const MAX_INBOUND_WEBSOCKET_MESSAGE_BYTES = 32 * 1024 * 1024; @@ -1291,6 +1291,325 @@ describe('websocket store unified resource contract', () => { } }); + describe('active alert payloads replace the stored alert', () => { + // A threshold alert held below its trigger, as the socket sends it: the + // breach stays in value/message and the reading evaluated now rides in + // metricStatus. + const heldTemperatureAlert = (): Record => ({ + id: 'pve2-temperature', + type: 'temperature', + level: 'warning', + resourceId: 'node:pve2', + resourceName: 'pve2', + node: 'pve2', + nodeDisplayName: 'PVE Two', + instance: 'pve2', + message: 'Node temperature at 95°C', + value: 95, + threshold: 80, + startTime: '2026-10-06T10:00:00Z', + acknowledged: false, + metadata: { resourceType: 'Node', unit: '°C', probe: 'cpu.package' }, + metricStatus: { + phase: 'latched', + value: 78, + observedAt: '2026-10-06T10:05:00Z', + trigger: 80, + recovery: 75, + unit: '°C', + }, + }); + + // The same alert as the backend restores it after a restart: no live + // status until the next evaluation. + const restoredTemperatureAlert = (): Record => { + const alert = heldTemperatureAlert(); + delete alert.metricStatus; + return alert; + }; + + const emitActiveAlerts = (lastUpdate: number, activeAlerts: unknown[]) => + emitMessage({ type: 'rawData', data: { lastUpdate, activeAlerts } }); + + const emitInitialAlerts = (activeAlerts: unknown[]) => + emitMessage({ + type: 'initialState', + data: { resources: [], lastUpdate: 100, activeAlerts, recentlyResolved: [] }, + }); + + it('drops a live status, display name and metadata that a snapshot omits', async () => { + const { store, dispose } = await createStoreHarness(); + try { + await waitForOpenTick(); + emitInitialAlerts([heldTemperatureAlert()]); + expect(store.activeAlerts['pve2-temperature']?.metricStatus?.value).toBe(78); + + emitActiveAlerts(200, [restoredTemperatureAlert()]); + const restored = store.activeAlerts['pve2-temperature']; + expect(restored).not.toHaveProperty('metricStatus'); + expect(restored?.value).toBe(95); + expect(store.state.activeAlerts[0]).not.toHaveProperty('metricStatus'); + + const renamed = restoredTemperatureAlert(); + delete renamed.nodeDisplayName; + renamed.metadata = { resourceType: 'Node', unit: '°C' }; + emitActiveAlerts(300, [renamed]); + expect(store.activeAlerts['pve2-temperature']).not.toHaveProperty('nodeDisplayName'); + expect(store.activeAlerts['pve2-temperature']?.metadata).toEqual({ + resourceType: 'Node', + unit: '°C', + }); + + delete renamed.metadata; + emitActiveAlerts(400, [renamed]); + expect(store.activeAlerts['pve2-temperature']).not.toHaveProperty('metadata'); + } finally { + dispose(); + } + }); + + it('drops a live status that a keyed delta deletes', async () => { + const { store, dispose } = await createStoreHarness(); + try { + await waitForOpenTick(); + emitInitialAlerts([heldTemperatureAlert()]); + + emitMessage({ + type: 'rawData', + data: { + lastUpdate: 200, + activeAlertsDelta: { + upserts: [{ id: 'pve2-temperature', metricStatus: null, nodeDisplayName: null }], + }, + }, + }); + const stored = store.activeAlerts['pve2-temperature']; + expect(stored).not.toHaveProperty('metricStatus'); + expect(stored).not.toHaveProperty('nodeDisplayName'); + expect(stored?.message).toBe('Node temperature at 95°C'); + } finally { + dispose(); + } + }); + + it('drops a live status that REST recovery omits', async () => { + const { store, dispose } = await createStoreHarness(); + try { + await waitForOpenTick(); + emitInitialAlerts([heldTemperatureAlert()]); + + apiFetchJSONMock.mockResolvedValueOnce([restoredTemperatureAlert()]); + await expect(store.refreshActiveAlerts()).resolves.toBe(true); + expect(apiFetchJSONMock).toHaveBeenCalledWith('/api/alerts/active'); + expect(store.activeAlerts['pve2-temperature']).not.toHaveProperty('metricStatus'); + } finally { + dispose(); + } + }); + + it('clears acknowledgement fields when the server reports an unacknowledge', async () => { + const { store, dispose } = await createStoreHarness(); + try { + await waitForOpenTick(); + emitInitialAlerts([ + { + ...restoredTemperatureAlert(), + acknowledged: true, + ackTime: '2026-10-06T10:06:00Z', + ackUser: 'admin', + }, + ]); + expect(store.activeAlerts['pve2-temperature']?.ackUser).toBe('admin'); + + // Unacknowledged from another session: the payload omits both fields. + emitActiveAlerts(200, [restoredTemperatureAlert()]); + const stored = store.activeAlerts['pve2-temperature']; + expect(stored?.acknowledged).toBe(false); + expect(stored).not.toHaveProperty('ackTime'); + expect(stored).not.toHaveProperty('ackUser'); + } finally { + dispose(); + } + }); + + it('holds a local acknowledge and unacknowledge until the server confirms each', async () => { + const { store, dispose } = await createStoreHarness(); + try { + await waitForOpenTick(); + const id = 'pve2-temperature'; + const acknowledged = { + ...restoredTemperatureAlert(), + acknowledged: true, + ackTime: '2026-10-06T10:06:01Z', + ackUser: 'admin', + }; + emitInitialAlerts([restoredTemperatureAlert()]); + + store.updateAlert(id, { + acknowledged: true, + ackTime: '2026-10-06T10:06:00Z', + ackUser: undefined, + }); + emitActiveAlerts(200, [restoredTemperatureAlert()]); + expect(store.activeAlerts[id]?.acknowledged).toBe(true); + + emitActiveAlerts(300, [acknowledged]); + expect(store.activeAlerts[id]).toMatchObject({ + acknowledged: true, + ackTime: '2026-10-06T10:06:01Z', + ackUser: 'admin', + }); + + store.updateAlert(id, { acknowledged: false, ackTime: undefined, ackUser: undefined }); + emitActiveAlerts(400, [acknowledged]); + expect(store.activeAlerts[id]?.acknowledged).toBe(false); + expect(store.activeAlerts[id]).not.toHaveProperty('ackUser'); + + emitActiveAlerts(500, [restoredTemperatureAlert()]); + expect(store.activeAlerts[id]?.acknowledged).toBe(false); + expect(store.activeAlerts[id]).not.toHaveProperty('ackTime'); + expect(store.activeAlerts[id]).not.toHaveProperty('ackUser'); + } finally { + dispose(); + } + }); + + it('keeps the record a local update replaced intact for rollback', async () => { + const { store, dispose } = await createStoreHarness(); + try { + await waitForOpenTick(); + const id = 'pve2-temperature'; + const record = { + id: 'alert:pve2-temperature', + canonicalSpecId: 'temperature', + subjectResourceId: 'node:pve2', + state: 'open', + severity: 'warning', + firstObservedAt: '2026-10-06T10:00:00Z', + lastObservedAt: '2026-10-06T10:05:00Z', + stateChangedAt: '2026-10-06T10:00:00Z', + acknowledgement: { at: '2026-10-06T10:01:00Z', by: 'admin' }, + evidenceIds: ['evidence-1'], + causeKey: 'pve2-temperature', + relatedResourceIds: [], + }; + emitInitialAlerts([{ ...restoredTemperatureAlert(), operationalRecord: record }]); + + // As a snooze does: the optimistic record reuses nested values of the + // record it keeps for rollback. + const previous = store.activeAlerts[id]?.operationalRecord; + if (!previous) throw new Error('expected an operational record'); + store.updateAlert(id, { + operationalRecord: { + ...previous, + state: 'suppressed', + suppression: { + at: '2026-10-06T10:06:00Z', + by: 'current-user', + reason: 'user_snooze', + expiresAt: '2026-10-06T11:06:00Z', + }, + }, + }); + + emitActiveAlerts(200, [ + { + ...restoredTemperatureAlert(), + operationalRecord: { + ...record, + acknowledgement: { at: '2026-10-06T10:07:00Z', by: 'operator' }, + evidenceIds: ['evidence-1', 'evidence-2'], + }, + }, + ]); + expect(store.activeAlerts[id]?.operationalRecord?.acknowledgement?.by).toBe('operator'); + expect(previous.acknowledgement).toEqual({ at: '2026-10-06T10:01:00Z', by: 'admin' }); + expect([...previous.evidenceIds]).toEqual(['evidence-1']); + } finally { + dispose(); + } + }); + + it('leaves unchanged alerts and unchanged nested fields quiet', async () => { + const { store, dispose } = await createStoreHarness(); + let disposeTracking = () => {}; + try { + await waitForOpenTick(); + const cpuAlert = (): Record => ({ + ...heldTemperatureAlert(), + id: 'pve1-cpu', + type: 'cpu', + resourceId: 'node:pve1', + metricStatus: { + phase: 'breaching', + value: 93, + observedAt: '2026-10-06T10:05:00Z', + trigger: 90, + recovery: 85, + }, + }); + emitInitialAlerts([heldTemperatureAlert(), cpuAlert()]); + + let temperatureRuns = 0; + let cpuRuns = 0; + createRoot((d) => { + disposeTracking = d; + createComputed(() => { + void store.activeAlerts['pve2-temperature']?.metricStatus?.value; + temperatureRuns += 1; + }); + createComputed(() => { + void store.activeAlerts['pve1-cpu']?.metricStatus?.value; + cpuRuns += 1; + }); + }); + const cpuBefore = store.activeAlerts['pve1-cpu']; + const cpuStatusBefore = cpuBefore?.metricStatus; + + const cooler = heldTemperatureAlert(); + cooler.metricStatus = { ...(cooler.metricStatus as object), value: 77 }; + emitActiveAlerts(200, [cooler, cpuAlert()]); + + expect(store.activeAlerts['pve2-temperature']?.metricStatus?.value).toBe(77); + expect(temperatureRuns).toBe(2); + expect(cpuRuns).toBe(1); + expect(store.activeAlerts['pve1-cpu']).toBe(cpuBefore); + expect(store.activeAlerts['pve1-cpu']?.metricStatus).toBe(cpuStatusBefore); + } finally { + disposeTracking(); + dispose(); + } + }); + + it('drops fields a recently resolved alert payload omits', async () => { + const { store, dispose } = await createStoreHarness(); + try { + await waitForOpenTick(); + const resolved = { + ...restoredTemperatureAlert(), + resolvedTime: '2026-10-06T10:10:00Z', + }; + emitMessage({ + type: 'initialState', + data: { + resources: [], + lastUpdate: 100, + activeAlerts: [], + recentlyResolved: [{ ...resolved, nodeDisplayName: 'PVE Two' }], + }, + }); + expect(store.recentlyResolved['pve2-temperature']?.nodeDisplayName).toBe('PVE Two'); + + const renamed: Record = { ...resolved }; + delete renamed.nodeDisplayName; + emitMessage({ type: 'rawData', data: { lastUpdate: 200, recentlyResolved: [renamed] } }); + expect(store.recentlyResolved['pve2-temperature']).not.toHaveProperty('nodeDisplayName'); + } finally { + dispose(); + } + }); + }); + it('drops scroll-queued deltas when a full snapshot supersedes their baseline', async () => { const { store, dispose } = await createStoreHarness(); try { diff --git a/frontend-modern/src/stores/websocket.ts b/frontend-modern/src/stores/websocket.ts index e3f1f34b11..28023d5c01 100644 --- a/frontend-modern/src/stores/websocket.ts +++ b/frontend-modern/src/stores/websocket.ts @@ -422,7 +422,12 @@ export function createWebSocketStore(url: string) { clearPendingAck(id); } - setActiveAlerts(id, alert); + // Each payload is the server's whole alert. A plain path set would + // shallow-merge it, so a field the server now omits (metricStatus on an + // alert restored after a restart, ackTime/ackUser after an unacknowledge) + // would keep its old value. reconcile removes omitted keys and diffs + // nested values in place, so unchanged fields stay quiet. + setActiveAlerts(id, reconcile(alert)); }); setState('activeAlerts', Object.values(alertsMap)); @@ -1380,9 +1385,10 @@ export function createWebSocketStore(url: string) { } }); - // Add new resolved alerts + // Add new resolved alerts, replacing rather than merging (see + // applyActiveAlerts) Object.entries(newResolvedAlerts).forEach(([id, alert]) => { - setRecentlyResolved(id, alert); + setRecentlyResolved(id, reconcile(alert)); }); setState('recentlyResolved', Object.values(newResolvedAlerts)); @@ -1973,7 +1979,13 @@ export function createWebSocketStore(url: string) { }, 15000); pendingAckTimeouts.set(alertIdentifier, pendingTimeout); } - setActiveAlerts(alertIdentifier, { ...existingAlert, ...updates }); + // Incoming payloads reconcile stored alerts in place, so store a copy + // of the update: an optimistic record can share nested objects with + // the record its caller keeps for rollback. + setActiveAlerts(alertIdentifier, { + ...existingAlert, + ...structuredClone(unwrap(updates)), + }); } }, };