From 495562ef663e473889490a774a80cf8d04405732 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Sun, 6 Sep 2026 17:12:21 +0100 Subject: [PATCH] fix(alerts): ignore superseded delivery diagnosis responses Overlapping bulk diagnosis refreshes could resolve out of order and replace a current notifications-disabled warning with older dispatch evidence. Version each request, including empty alert sets, so only the newest response updates card diagnoses. Pin overlap and empty-set invalidation with component and registered hook regressions. All 47 focused tests and TypeScript pass. Isolated Chromium verifies rendered ordering at three widths; update both subsystem contracts and bind browser proof to the runtime bytes. No backend delivery or recipient receipt is claimed. Change-source: pulse-maintainer --- .../v6/internal/subsystems/alerts.md | 9 ++ .../subsystems/frontend-primitives.md | 9 ++ frontend-modern/browser-verification.json | 12 +- .../OverviewTab.deliverystatus.test.tsx | 22 +++ .../__tests__/useAlertOverviewState.test.tsx | 29 +++- .../features/alerts/useAlertOverviewState.ts | 6 +- scripts/check-alert-diagnosis-ordering.mjs | 146 ++++++++++++++++++ 7 files changed, 225 insertions(+), 8 deletions(-) create mode 100644 scripts/check-alert-diagnosis-ordering.mjs diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index a47e21132..acb62df48 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -15,6 +15,15 @@ ## Purpose +Overview delivery diagnoses use latest-started refresh ownership. Older bulk +responses cannot overwrite newer card notification status, and an empty active +alert set invalidates outstanding reads. Disposal also prevents updates. Failed +refreshes retain the existing snapshot; this ordering repair does not add a +freshness indicator or establish recipient receipt. Verify response overlap in +`OverviewTab.deliverystatus.test.tsx`, empty-set invalidation in +`useAlertOverviewState.test.tsx`, and rendered ordering at three widths using +`scripts/check-alert-diagnosis-ordering.mjs`. + Delivery-attempt and held-event reads in Destinations use latest-started refresh ownership. A delayed mount response must not overwrite evidence from configuration Retry or a queue-action refresh, including a newer unavailable diff --git a/docs/release-control/v6/internal/subsystems/frontend-primitives.md b/docs/release-control/v6/internal/subsystems/frontend-primitives.md index 19d0e4991..7af5c7136 100644 --- a/docs/release-control/v6/internal/subsystems/frontend-primitives.md +++ b/docs/release-control/v6/internal/subsystems/frontend-primitives.md @@ -20,6 +20,15 @@ ## Purpose +Overview delivery diagnoses use latest-started refresh ownership. Older bulk +responses cannot overwrite newer card notification status, and an empty active +alert set invalidates outstanding reads. Disposal also prevents updates. Failed +refreshes retain the existing snapshot; this ordering repair does not add a +freshness indicator or establish recipient receipt. Verify response overlap in +`OverviewTab.deliverystatus.test.tsx`, empty-set invalidation in +`useAlertOverviewState.test.tsx`, and rendered ordering at three widths using +`scripts/check-alert-diagnosis-ordering.mjs`. + The Destinations delivery-log state primitive assigns a generation to each refresh and rejects stale completions before updating rows, unavailable state or loading state. Held-event reads share that generation without blocking the diff --git a/frontend-modern/browser-verification.json b/frontend-modern/browser-verification.json index 5e8b35ad1..1fd431dcf 100644 --- a/frontend-modern/browser-verification.json +++ b/frontend-modern/browser-verification.json @@ -1,13 +1,13 @@ { "version": 1, - "base_sha": "4fd68c72b26f033f5b1b0fcb7d7b2b0d67e85ec8", - "verified_at": "2026-09-06T14:12:06.315693Z", + "base_sha": "5000e0409b5795f732f32882d7c6929e7eff16b1", + "verified_at": "2026-09-06T16:16:58.486143Z", "result": "passed", "changed_paths": [ - "frontend-modern/src/features/alerts/deliveryDiagnosisPresentation.ts" + "frontend-modern/src/features/alerts/useAlertOverviewState.ts" ], "content_sha256": { - "frontend-modern/src/features/alerts/deliveryDiagnosisPresentation.ts": "bc3eadf8f790517b377430a42eb67e8fdfcadb53bb13b87cd971d6a9ab91d607" + "frontend-modern/src/features/alerts/useAlertOverviewState.ts": "64d0b891e7ad228e8590da859dc25e825b6164c8cf76a01983a219d6cd079b23" }, "routes": [ "/qualification (isolated real OverviewTab, not installed /alerts)" @@ -27,9 +27,9 @@ } ], "states": [ - "Scripted ready and cooldown diagnoses with lastNotified render Dispatch requested; cooldown says next eligible. Missing timestamp remains Notification pending. No Notified label." + "First diagnosis request pending; active set expanded; newer notifications-disabled response visible; older dispatch response completes without replacing current status." ], "interactions": [ - "Loaded real Overview in Chromium via scripts/check-alert-dispatch-copy.mjs; asserted status text ranges fit each viewport and no page errors. Inspected desktop and phone screenshots. No delivery actions invoked; no backend receipt claimed. Screenshots retained in lane outcome evidence." + "Ran pulse-heavy-run -- node scripts/check-alert-diagnosis-ordering.mjs in Chromium. Clicked Add alert fixture control, completed older response after current warning rendered, asserted warning retained, dispatch absent, new alert present, status text fits viewport and no page errors. Inspected desktop and phone screenshots. Synthetic API only; no delivery action or recipient receipt." ] } diff --git a/frontend-modern/src/features/alerts/__tests__/OverviewTab.deliverystatus.test.tsx b/frontend-modern/src/features/alerts/__tests__/OverviewTab.deliverystatus.test.tsx index 631ae1590..5dcaaaeb7 100644 --- a/frontend-modern/src/features/alerts/__tests__/OverviewTab.deliverystatus.test.tsx +++ b/frontend-modern/src/features/alerts/__tests__/OverviewTab.deliverystatus.test.tsx @@ -1,4 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { createSignal } from 'solid-js'; import { cleanup, render, screen, waitFor } from '@solidjs/testing-library'; import { DEFAULT_LOCALE, setActiveLocale } from '@/i18n'; import type { Alert, AlertDeliveryDiagnosis } from '@/types/api'; @@ -124,6 +125,27 @@ describe('OverviewTab delivery status line', () => { if (state.reason === 'cooldown') expect(screen.getByText(/next eligible/)).toBeTruthy(); }); + it('ignores an older diagnosis response after the active alert set changes', async () => { + let finishOlder!: (value: AlertDeliveryDiagnosis[]) => void; + getDeliveryDiagnoses.mockReturnValueOnce( + new Promise((resolve) => { + finishOlder = resolve; + }), + ); + getDeliveryDiagnoses.mockResolvedValueOnce([ + makeDiagnosis('a1', { status: 'suppressed', reason: 'notifications_disabled' }), + ]); + const [alerts, setAlerts] = createSignal>({ a1: makeAlert('a1') }); + render(() => ); + await waitFor(() => expect(getDeliveryDiagnoses).toHaveBeenCalledTimes(1)); + setAlerts({ a1: makeAlert('a1'), a2: makeAlert('a2') }); + await waitFor(() => expect(screen.getByText('Notifications are turned off')).toBeTruthy()); + finishOlder([makeDiagnosis('a1', { lastNotified: '2026-08-26T10:15:00Z' })]); + await Promise.resolve(); + expect(screen.queryByText(/^Dispatch requested /)).toBeNull(); + expect(screen.getByText('Notifications are turned off')).toBeTruthy(); + }); + it('renders no delivery line when the diagnosis fetch fails', async () => { const activeAlerts: Record = { a1: makeAlert('a1') }; getDeliveryDiagnoses.mockRejectedValue(new Error('boom')); diff --git a/frontend-modern/src/features/alerts/__tests__/useAlertOverviewState.test.tsx b/frontend-modern/src/features/alerts/__tests__/useAlertOverviewState.test.tsx index edc4f4569..2b3a96ddc 100644 --- a/frontend-modern/src/features/alerts/__tests__/useAlertOverviewState.test.tsx +++ b/frontend-modern/src/features/alerts/__tests__/useAlertOverviewState.test.tsx @@ -4,12 +4,13 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { AlertsAPI } from '@/api/alerts'; import { notificationStore } from '@/stores/notifications'; -import type { Alert } from '@/types/api'; +import type { Alert, AlertDeliveryDiagnosis } from '@/types/api'; import { useAlertOverviewState } from '../useAlertOverviewState'; vi.mock('@/api/alerts', () => ({ AlertsAPI: { + getDeliveryDiagnoses: vi.fn(), acknowledge: vi.fn(), bulkAcknowledge: vi.fn(), unacknowledge: vi.fn(), @@ -47,6 +48,7 @@ describe('useAlertOverviewState', () => { beforeEach(() => { vi.useFakeTimers(); vi.setSystemTime(new Date('2026-03-22T12:00:00Z')); + vi.mocked(AlertsAPI.getDeliveryDiagnoses).mockReset().mockResolvedValue([]); vi.mocked(AlertsAPI.acknowledge).mockReset(); vi.mocked(AlertsAPI.unacknowledge).mockReset(); vi.mocked(AlertsAPI.bulkAcknowledge).mockReset(); @@ -58,6 +60,31 @@ describe('useAlertOverviewState', () => { vi.useRealTimers(); }); + it('invalidates pending diagnosis reads when the active set becomes empty', async () => { + let finish!: (value: AlertDeliveryDiagnosis[]) => void; + vi.mocked(AlertsAPI.getDeliveryDiagnoses).mockReturnValueOnce( + new Promise((resolve) => { + finish = resolve; + }), + ); + const [activeAlerts, setActiveAlerts] = createSignal>({ + a1: makeAlert('a1', new Date().toISOString()), + }); + const { result } = renderHook(() => + useAlertOverviewState({ + activeAlerts, + overrides: () => [], + showAcknowledged: () => true, + updateAlert: vi.fn(), + }), + ); + expect(AlertsAPI.getDeliveryDiagnoses).toHaveBeenCalledOnce(); + setActiveAlerts({}); + finish([{ alertIdentifier: 'a1', reason: 'ready' } as AlertDeliveryDiagnosis]); + await Promise.resolve(); + expect(result.deliveryDiagnoses()).toEqual({}); + }); + it('owns overview stats, filtering, and acknowledge flows outside the tab shell', async () => { const now = Date.now(); const [activeAlerts] = createSignal>({ diff --git a/frontend-modern/src/features/alerts/useAlertOverviewState.ts b/frontend-modern/src/features/alerts/useAlertOverviewState.ts index 650a92063..377bb479e 100644 --- a/frontend-modern/src/features/alerts/useAlertOverviewState.ts +++ b/frontend-modern/src/features/alerts/useAlertOverviewState.ts @@ -68,17 +68,21 @@ export function useAlertOverviewState(props: UseAlertOverviewStateProps) { Record >({}); let diagnosisStateDisposed = false; + let diagnosisRequestVersion = 0; onCleanup(() => { diagnosisStateDisposed = true; }); const refreshDeliveryDiagnoses = async () => { + // A slower previous refresh must not replace a newer notification state. + // Increment even for an empty alert set to invalidate outstanding requests. + const requestVersion = ++diagnosisRequestVersion; if (activeAlerts().length === 0) { setDeliveryDiagnoses({}); return; } try { const list = await AlertsAPI.getDeliveryDiagnoses(); - if (diagnosisStateDisposed) return; + if (diagnosisStateDisposed || requestVersion !== diagnosisRequestVersion) return; const next: Record = {}; for (const diagnosis of list) { next[diagnosis.alertIdentifier || diagnosis.alertId] = diagnosis; diff --git a/scripts/check-alert-diagnosis-ordering.mjs b/scripts/check-alert-diagnosis-ordering.mjs new file mode 100644 index 000000000..45b4e30f4 --- /dev/null +++ b/scripts/check-alert-diagnosis-ordering.mjs @@ -0,0 +1,146 @@ +// Isolated real-browser component qualification; no installed backend or delivery claim. +import { createServer } from "../frontend-modern/node_modules/vite/dist/node/index.js"; +import solid from "../frontend-modern/node_modules/vite-plugin-solid/dist/esm/index.mjs"; +import { chromium } from "@playwright/test"; +import { resolve } from "node:path"; +import { mkdirSync } from "node:fs"; +import assert from "node:assert/strict"; +const root = resolve("frontend-modern"); +process.chdir(root); +const fixture = ` +import { createSignal } from 'solid-js'; +import { render } from 'solid-js/web'; +import { Router, Route } from '@solidjs/router'; +import { AlertsAPI } from '/src/api/alerts'; +import { NotificationsAPI } from '/src/api/notifications'; +import { OverviewTab } from '/src/features/alerts/OverviewTab'; +import '/src/index.css'; +let finishOlder; +let requests = 0; +AlertsAPI.getDeliveryDiagnoses = () => { + requests++; + if (requests === 1) return new Promise(resolve => { finishOlder = resolve; }); + return Promise.resolve([{alertIdentifier:'a1', alertId:'a1', status:'suppressed', + reason:'notifications_disabled', message:'Notifications disabled by current configuration'}]); +}; +window.finishOlder = () => finishOlder([{alertIdentifier:'a1', alertId:'a1', + status:'would_send',reason:'ready',lastNotified:'2026-08-26T10:15:00Z'}]); +window.requestCount = () => requests; +AlertsAPI.getEvents = async () => []; +NotificationsAPI.getHealth = async () => ({queue:{status:'healthy'}}); +const alert = id => ({id,resourceId:id,resourceName:'VM '+id,type:'cpu',level:'warning', +message:'High CPU on '+id,startTime:new Date().toISOString(),acknowledged:false,node:'node1'}); +function Fixture() { +const [alerts, setAlerts] = createSignal({a1:alert('a1')}); +return
+{}} showQuickTip={()=>false} dismissQuickTip={()=>{}} showAcknowledged={()=>true} +setShowAcknowledged={()=>{}} alertsDisabled={()=>false}/>
; } +render(()=>,document.getElementById('root')); +`; +const server = await createServer({ + root, + configFile: false, + optimizeDeps: { + noDiscovery: true, + entries: [], + esbuildOptions: { target: "esnext" }, + }, + esbuild: { target: "esnext" }, + plugins: [ + solid(), + { + name: "dispatch-fixture", + configureServer(s) { + s.middlewares.use((req, res, next) => { + if (req.url === "/qualification") { + res.setHeader("Content-Type", "text/html"); + res.end( + '
', + ); + } else next(); + }); + }, + resolveId(id) { + if (id === "/dispatch-fixture.tsx") return id; + }, + load(id) { + if (id === "/dispatch-fixture.tsx") return fixture; + }, + }, + ], + resolve: { alias: { "@": resolve(root, "src") } }, + server: { host: "127.0.0.1", port: 5199, strictPort: true }, +}); +let browser; +try { + await server.listen(); + browser = await chromium.launch({ headless: true }); + mkdirSync("/tmp/pulse-alert-diagnosis-ordering", { recursive: true }); + for (const width of [1440, 900, 390]) { + const page = await browser.newPage({ viewport: { width, height: 1000 } }); + const errors = []; + page.on("pageerror", (e) => { + errors.push(e.message); + console.error(e.message); + }); + page.on("console", (m) => { + if (m.type() === "error") console.error(m.text()); + }); + await page.route("http://127.0.0.1:5199/api/**", (route) => + route.fulfill({ json: [] }), + ); + await page.goto("http://127.0.0.1:5199/qualification"); + await page.waitForFunction(() => window.requestCount?.() === 1); + await page.getByRole("button", { name: "Add alert", exact: true }).click(); + await page + .getByText("Notifications are turned off", { exact: true }) + .waitFor(); + await page.evaluate(async () => { + window.finishOlder(); + await Promise.resolve(); + }); + assert.equal( + await page + .getByText("Notifications are turned off", { exact: true }) + .count(), + 1, + ); + assert.equal(await page.getByText(/^Dispatch requested /).count(), 0); + assert.equal( + await page.getByText("High CPU on a2", { exact: true }).count(), + 1, + ); + const label = page.getByText("Notifications are turned off", { + exact: true, + }); + assert.equal( + await label.evaluate((el) => { + const range = document.createRange(); + range.selectNodeContents(el); + return [...range.getClientRects()].every( + (b) => b.left >= 0 && b.right <= innerWidth, + ); + }), + true, + "current status must fit viewport", + ); + assert.deepEqual(errors, []); + await page.screenshot({ + path: "/tmp/pulse-alert-diagnosis-ordering/" + width + ".png", + fullPage: true, + }); + await page.close(); + } + console.log( + JSON.stringify({ + result: "passed", + viewports: [1440, 900, 390], + scope: + "Real Overview and Chromium; scripted diagnoses, not installed delivery or receipt", + }), + ); +} finally { + await browser?.close(); + await server.close(); +}