From c27c8abc77cb5cf9c5b35765aa95395c9edaf46f Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Sun, 6 Sep 2026 10:19:46 +0100 Subject: [PATCH] fix(web): retain current delivery evidence across overlapping reads Mount and Retry reads can finish out of order, erasing current attempts and held-event evidence or hiding an unavailable result. Assign refresh ownership and ignore abandoned completions after disposal. Ordinary regression tests and six scripted Chromium cases protect ordering and loading state; this does not qualify installed notification delivery. Change-source: pulse-maintainer --- .../v6/internal/subsystems/alerts.md | 11 ++ .../v6/internal/subsystems/api-contracts.md | 11 ++ .../subsystems/frontend-primitives.md | 12 ++ frontend-modern/browser-verification.json | 42 ++---- .../useAlertDestinationsTabState.test.tsx | 47 +++++++ .../useNotificationDeliveryLog.test.tsx | 74 ++++++++++- .../alerts/useNotificationDeliveryLog.ts | 23 +++- scripts/check-delivery-log-ordering.mjs | 125 ++++++++++++++++++ 8 files changed, 301 insertions(+), 44 deletions(-) create mode 100644 scripts/check-delivery-log-ordering.mjs diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index 8eb13ba3c..ed5a3b0e3 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -15,6 +15,17 @@ ## Purpose +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 +result. Attempt loading reflects only the latest attempt request; held events +remain independently asynchronous. Scope disposal abandons both reads. +Verification: ordinary overlap, held-event and disposal cases in +`useNotificationDeliveryLog.test.tsx`, registered mount/Retry coverage in +`useAlertDestinationsTabState.test.tsx`, and positive rendered-content assertions +in `scripts/check-delivery-log-ordering.mjs`. Scripted browser evidence is not +proof of backend delivery or receipt by an independent recipient. + The shared delivery-health card wraps action groups according to available space, retaining readable explanation width when Review, Retry, Dismiss and Refresh appear together. Its heading uses the opaque semantic foreground, diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index 0449a97aa..fcb1d3e1f 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -4774,6 +4774,17 @@ requests only the two held-notification event types over the same seven-day window as its delivery-attempt log, with an explicit bounded limit. That event request runs independently of `GET /api/notifications/delivery-log`; failure of either API must not recast the other response as empty or unavailable. +Overlapping Destinations mount, configuration Retry and queue-action refreshes +use latest-request ownership: only the most recently started refresh may +replace delivery attempts, unavailable/loading flags or held-event rows. +An older success cannot hide a newer unavailable result, and an older failure +cannot erase newer evidence. Held-event completion remains independent of the +attempt-log loading flag. Disposing the owning Solid scope ignores pending +completions and prevents further reads through the abandoned loader. +Regression coverage lives in the delivery-log and destinations-tab state tests; +`scripts/check-delivery-log-ordering.mjs` additionally asserts positive +rendered current-attempt content in Chromium at desktop and mobile widths +using scripted APIs, not installed delivery or recipient evidence. `GET /api/alerts/incidents` with both `alertIdentifier` and `started_at` is an occurrence-qualified timeline read. The incident/resource-history projection diff --git a/docs/release-control/v6/internal/subsystems/frontend-primitives.md b/docs/release-control/v6/internal/subsystems/frontend-primitives.md index 0b217c234..47f916c66 100644 --- a/docs/release-control/v6/internal/subsystems/frontend-primitives.md +++ b/docs/release-control/v6/internal/subsystems/frontend-primitives.md @@ -20,6 +20,18 @@ ## Purpose +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 +attempt-log spinner. Cleanup prevents abandoned requests from updating state +and makes subsequent calls through the disposed loader inert; it does not +cancel transport requests. Latest-request failure remains unavailable rather +than being concealed by an older successful response. +Verification combines the hook's ordinary race/disposal tests, registered +mount/Retry integration, and `scripts/check-delivery-log-ordering.mjs` Chromium +content assertions at desktop and narrow widths. This is component-level +presentation proof with scripted APIs, not full-tab or installed qualification. + The shared delivery-health card wraps action groups according to available space, retaining readable explanation width when Review, Retry, Dismiss and Refresh appear together. Its heading uses the opaque semantic foreground, diff --git a/frontend-modern/browser-verification.json b/frontend-modern/browser-verification.json index df645715b..8e19c26a0 100644 --- a/frontend-modern/browser-verification.json +++ b/frontend-modern/browser-verification.json @@ -1,57 +1,33 @@ { "version": 1, - "base_sha": "61607333cc9e2fe1c1712b94ce458ac45abdabd1", - "verified_at": "2026-09-06T03:17:28.710286Z", + "base_sha": "9761e5e91457a2d007542ff61074db9ddcc427e7", + "verified_at": "2026-09-06T09:19:36.187652Z", "result": "passed", "changed_paths": [ - "frontend-modern/src/features/alerts/AlertDeliveryHealthCard.tsx", - "frontend-modern/src/features/alerts/OverviewTab.tsx", - "frontend-modern/src/features/alerts/useNotificationDeliveryHealth.ts" + "frontend-modern/src/features/alerts/useNotificationDeliveryLog.ts" ], "content_sha256": { - "frontend-modern/src/features/alerts/AlertDeliveryHealthCard.tsx": "dad60838804e575c0314398b304bda6d2fbd842cb2f693b3f33d168a4bce5dc1", - "frontend-modern/src/features/alerts/OverviewTab.tsx": "8f7fdc04bd0546f86152c8bb392ff3e3fb755f1f0a1c28d1ee85da3e9fe23582", - "frontend-modern/src/features/alerts/useNotificationDeliveryHealth.ts": "1eadf6df30b4f1130f868b8f139561ea60fdb87e1823deeeaaf2f8fbc699a00c" + "frontend-modern/src/features/alerts/useNotificationDeliveryLog.ts": "be919497714d749fcbe9a48a325aa82282b7afa8d6866d84eb1cc09ab52c14a6" }, "routes": [ - "/qualification (actual delivery-health and Overview components in isolated Solid Router fixtures, scripted API promises)", - "/patrol", - "/alerts" + "/qualification (real Destinations state hook and delivery-log card, scripted APIs; not full DestinationsTab)" ], "viewports": [ { "width": 1440, "height": 900 }, - { - "width": 900, - "height": 900 - }, { "width": 390, "height": 900 - }, - { - "width": 1440, - "height": 1000 - }, - { - "width": 900, - "height": 1000 - }, - { - "width": 390, - "height": 1000 } ], "states": [ - "Delivery ordering: 18 scripted cases at three widths. Overview refresh: 12 scripted cases across light/dark themes and three widths. Unavailable health, retained Retry/Dismiss actions, pending disabled Refresh and healthy recovery. These fixtures do not qualify installed backend delivery or recipient outcomes.", - "Final card pixels inspected at desktop, intermediate and narrow widths in both themes. Heading is readable and actions wrap within the card. This is a focused card check, not a full dark-theme shell audit.", - "Ordinary and alert-mirrored Patrol findings preserve unknown cause and failed-read evidence in the investigation transcript and linked Assistant. Scripted responses qualify presentation and context preservation, not diagnostic competence." + "Four cases preserve current attempt and held-event rows after stale success/failure; two cases retain disabled refresh while latest read is pending and show unavailable after latest failure. Held-event pending does not block attempt refresh." ], "interactions": [ - "Retry and Dismiss followed by failed health read, manual Refresh while pending, healthy recovery and overlapping old/new response ordering. Geometry assertions check heading overflow and control bounds.", - "Selected issue, evidence expansion, keyboard Review and Explain, nested transcript expansion/collapse, Assistant streaming, preserved draft, reload, error/retry, menu Escape and outside dismissal at 1440/900/390 widths.", - "Private receipts: /Volumes/Development/pulse/tmp/patrol-merge-browser/final-health-ordering and final-overview-refresh, and tmp/patrol-assistant-journey/result.json. No infrastructure mutations or autonomous provider calls." + "Registered mount read, fixture Configuration Retry invoking real handleRetry, scripted out-of-order attempt and held-event completion. Six Chromium cases passed via pulse-heavy-run -- node scripts/check-delivery-log-ordering.mjs.", + "Inspected 390-error.png and 1440-pending-unavailable.png: current attempt and held rows visible on narrow layout; unavailable warning visible at desktop. Synthetic component fixture only, no backend or independent-recipient qualification.", + "Evidence: queue/staging/20260906T091009Z-web-product/evidence/browser.log and screenshots under /var/lib/pulse-maintainer." ] } diff --git a/frontend-modern/src/features/alerts/__tests__/useAlertDestinationsTabState.test.tsx b/frontend-modern/src/features/alerts/__tests__/useAlertDestinationsTabState.test.tsx index 1982eb681..b01d8e43b 100644 --- a/frontend-modern/src/features/alerts/__tests__/useAlertDestinationsTabState.test.tsx +++ b/frontend-modern/src/features/alerts/__tests__/useAlertDestinationsTabState.test.tsx @@ -115,6 +115,53 @@ describe('useAlertDestinationsTabState', () => { expect(result.deliveryNeedsAttention()).toBe(true); }); + it('preserves Retry log evidence when the registered mount read fails late', async () => { + let rejectMount!: (error: Error) => void; + const current = { + entries: [], + windowDays: 30, + completedRetentionDays: 7, + deadLetterRetentionDays: 30, + }; + vi.mocked(NotificationsAPI.getWebhooks).mockResolvedValue([]); + vi.mocked(NotificationsAPI.getHealth).mockResolvedValue({ + queue: { status: 'healthy' }, + } as never); + vi.mocked(NotificationsAPI.getDeliveryLog) + .mockReturnValueOnce( + new Promise((_resolve, reject) => { + rejectMount = reject; + }), + ) + .mockResolvedValueOnce(current); + const [appriseConfig, setAppriseConfig] = createSignal(buildAppriseConfig()); + const onRetryLoad = vi.fn(); + const { result, cleanup } = renderHook(() => + useAlertDestinationsTabState({ + appriseConfig, + setAppriseConfig, + configLoadError: () => 'configuration unavailable', + emailConfig: () => buildEmailConfig(), + isLoadingDestinations: () => false, + isRetrying: () => false, + onRetryLoad, + }), + ); + try { + await waitFor(() => expect(NotificationsAPI.getDeliveryLog).toHaveBeenCalledTimes(1)); + result.handleRetry(); + await waitFor(() => expect(result.deliveryLog()).toEqual(current)); + expect(onRetryLoad).toHaveBeenCalledTimes(1); + rejectMount(new Error('stale mount failure')); + await Promise.resolve(); + expect(result.deliveryLog()).toEqual(current); + expect(result.deliveryLogUnavailable()).toBe(false); + expect(result.refreshingDeliveryLog()).toBe(false); + } finally { + cleanup(); + } + }); + beforeEach(() => { vi.mocked(AlertsAPI.getEvents).mockReset(); vi.mocked(AlertsAPI.getEvents).mockResolvedValue([]); diff --git a/frontend-modern/src/features/alerts/__tests__/useNotificationDeliveryLog.test.tsx b/frontend-modern/src/features/alerts/__tests__/useNotificationDeliveryLog.test.tsx index d7b110d3d..3eb17d6bd 100644 --- a/frontend-modern/src/features/alerts/__tests__/useNotificationDeliveryLog.test.tsx +++ b/frontend-modern/src/features/alerts/__tests__/useNotificationDeliveryLog.test.tsx @@ -113,10 +113,7 @@ describe('useNotificationDeliveryLog', () => { } })); - // Known defect: mount and queue-action refreshes can overlap. Keep the - // desired invariant executable until the governed runtime repair lands; - // Vitest fails these tests if the invariant starts passing unexpectedly. - it.fails('keeps the newest successful read when an older read fails', () => + it('keeps the newest successful read when an older read fails', () => createRoot(async (dispose) => { const older = deferred(); vi.mocked(NotificationsAPI.getDeliveryLog) @@ -135,7 +132,7 @@ describe('useNotificationDeliveryLog', () => { } })); - it.fails('does not replace a newer unavailable result with an older success', () => + it('does not replace a newer unavailable result with an older success', () => createRoot(async (dispose) => { const older = deferred(); vi.mocked(NotificationsAPI.getDeliveryLog) @@ -154,7 +151,7 @@ describe('useNotificationDeliveryLog', () => { } })); - it.fails('keeps refreshing true while the newest read remains pending', () => + it('keeps refreshing true while the newest read remains pending', () => createRoot(async (dispose) => { const older = deferred(); const newer = deferred(); @@ -175,4 +172,69 @@ describe('useNotificationDeliveryLog', () => { } })); + it.each(['success', 'failure'])('ignores an older held-event %s after a newer read', (outcome) => + createRoot(async (dispose) => { + const older = deferred>>(); + const current = [ + { + id: 2, + type: 'notification_deferred', + alertId: 'current', + occurredAt: '2026-09-06T08:00:00Z', + }, + ]; + vi.mocked(AlertsAPI.getEvents) + .mockReturnValueOnce(older.promise) + .mockResolvedValueOnce(current); + vi.mocked(NotificationsAPI.getDeliveryLog).mockResolvedValue(emptyLog); + const state = useNotificationDeliveryLog(); + try { + await state.loadDeliveryLog(); + await state.loadDeliveryLog(); + if (outcome === 'success') older.resolve([]); + else older.reject(new Error('stale held read')); + await Promise.resolve(); + expect(state.heldEvents()).toEqual(current); + } finally { + dispose(); + } + }), + ); + + it.each(['success', 'failure'])( + 'ignores pending %s after disposal and starts no new reads', + (outcome) => + createRoot(async (dispose) => { + const log = deferred(); + const held = deferred>>(); + vi.mocked(NotificationsAPI.getDeliveryLog).mockReturnValue(log.promise); + vi.mocked(AlertsAPI.getEvents).mockReturnValue(held.promise); + const state = useNotificationDeliveryLog(); + const pending = state.loadDeliveryLog(); + dispose(); + if (outcome === 'success') { + log.resolve(emptyLog); + held.resolve([ + { + id: 1, + type: 'notification_suppressed', + alertId: 'late', + occurredAt: '2026-09-06T08:00:00Z', + }, + ]); + } else { + log.reject(new Error('late log')); + held.reject(new Error('late held')); + } + await pending; + await state.loadDeliveryLog(); + expect(state.deliveryLog()).toBeNull(); + expect(state.deliveryLogUnavailable()).toBe(false); + expect(state.heldEvents()).toEqual([]); + // Disposal freezes state, including loading; no abandoned completion owns it. + expect(state.refreshingDeliveryLog()).toBe(true); + expect(NotificationsAPI.getDeliveryLog).toHaveBeenCalledTimes(1); + expect(AlertsAPI.getEvents).toHaveBeenCalledTimes(1); + }), + ); }); diff --git a/frontend-modern/src/features/alerts/useNotificationDeliveryLog.ts b/frontend-modern/src/features/alerts/useNotificationDeliveryLog.ts index 648ea2263..1ddceb830 100644 --- a/frontend-modern/src/features/alerts/useNotificationDeliveryLog.ts +++ b/frontend-modern/src/features/alerts/useNotificationDeliveryLog.ts @@ -1,4 +1,4 @@ -import { createSignal } from 'solid-js'; +import { createSignal, onCleanup } from 'solid-js'; import { AlertsAPI } from '@/api/alerts'; import { NotificationsAPI, type NotificationDeliveryLog } from '@/api/notifications'; @@ -26,7 +26,15 @@ export function useNotificationDeliveryLog() { const [refreshingDeliveryLog, setRefreshingDeliveryLog] = createSignal(false); const [heldEvents, setHeldEvents] = createSignal([]); - const loadHeldEvents = async () => { + // Both reads belong to the same refresh, but held events never block the log. + let latestRequest = 0; + let disposed = false; + onCleanup(() => { + disposed = true; + }); + const ownsRequest = (request: number) => !disposed && request === latestRequest; + + const loadHeldEvents = async (request: number) => { try { const since = new Date( Date.now() - HELD_EVENT_WINDOW_DAYS * 24 * 60 * 60 * 1000, @@ -36,31 +44,36 @@ export function useNotificationDeliveryLog() { since, limit: HELD_EVENT_LIMIT, }); - setHeldEvents(events); + if (ownsRequest(request)) setHeldEvents(events); } catch (error) { + if (!ownsRequest(request)) return; logger.error('Failed to load held alert notification events', error); setHeldEvents([]); } }; const loadDeliveryLog = async () => { + if (disposed) return; + const request = ++latestRequest; setRefreshingDeliveryLog(true); // Held events refresh independently: they must never delay or fail the // primary delivery-attempt log. - void loadHeldEvents(); + void loadHeldEvents(request); try { // Request the server's bounded maximum. A degraded queue can retain more // than the default page of 50 failures, and the evidence view should not // hide them behind unrelated successful attempts when space is available. const log = await NotificationsAPI.getDeliveryLog(DELIVERY_LOG_LIMIT); + if (!ownsRequest(request)) return; setDeliveryLog(log); setDeliveryLogUnavailable(false); } catch (error) { + if (!ownsRequest(request)) return; logger.error('Failed to load notification delivery log', error); setDeliveryLog(null); setDeliveryLogUnavailable(true); } finally { - setRefreshingDeliveryLog(false); + if (ownsRequest(request)) setRefreshingDeliveryLog(false); } }; diff --git a/scripts/check-delivery-log-ordering.mjs b/scripts/check-delivery-log-ordering.mjs new file mode 100644 index 000000000..877480124 --- /dev/null +++ b/scripts/check-delivery-log-ordering.mjs @@ -0,0 +1,125 @@ +// 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, expect } from "@playwright/test"; +import { fileURLToPath } from "node:url"; +import { resolve } from "node:path"; +import { mkdirSync } from "node:fs"; +import assert from "node:assert/strict"; +const root = fileURLToPath(new URL('../frontend-modern', import.meta.url)); +const evidence = process.env.PULSE_BROWSER_EVIDENCE_DIR || resolve(root, '../tmp/delivery-log-ordering'); +mkdirSync(evidence, { recursive: true }); +process.chdir(root); +const fixture = ` +import { render } from 'solid-js/web'; +import { NotificationsAPI } from '/src/api/notifications'; +import { AlertsAPI } from '/src/api/alerts'; +import { useAlertDestinationsTabState } from '/src/features/alerts/useAlertDestinationsTabState'; +import { AlertDeliveryLogCard } from '/src/features/alerts/AlertDeliveryLogCard'; +import '/src/index.css'; +const pending = []; +const held = []; +NotificationsAPI.getHealth = async () => ({queue:{status:'healthy'}}); +NotificationsAPI.getDeliveryLog = () => new Promise((resolve, reject) => pending.push({resolve, reject})); +AlertsAPI.getEvents = () => new Promise((resolve, reject) => held.push({resolve, reject})); +window.finishHeld = (i, status) => status === 'error' ? held[i].reject(new Error('scripted held failure')) : held[i].resolve([{id:i+1,type:'notification_deferred',alertId:status,occurredAt:'2026-09-06T08:01:00Z'}]); +NotificationsAPI.retryTerminalFailures = NotificationsAPI.dismissTerminalFailures = async () => ({affected: 1}); +window.confirm = () => true; +window.finish = (i, status) => status === 'error' ? pending[i].reject(new Error('scripted offline')) : pending[i].resolve({entries:[{notificationId:status,type:'email',outcome:'sent',alertIds:[status],alertCount:1,attempts:1,success:true,timestamp:'2026-09-06T08:00:00Z'}],windowDays:30,completedRetentionDays:7,deadLetterRetentionDays:30}); +window.count = () => pending.length; +function Fixture() { +const s = useAlertDestinationsTabState({emailConfig:()=>({}), appriseConfig:()=>({}), setAppriseConfig:()=>{}, configLoadError:()=> 'scripted config unavailable', isRetrying:()=>false, isLoadingDestinations:()=>false, onRetryLoad:()=>{}, webhooks:()=>[]}); +return

Delivery log ordering fixture

{String(s.refreshingDeliveryLog())}
; +} +render(() => , document.getElementById('root')); +`; +const server = await createServer({ + root, + configFile: false, + optimizeDeps: { + noDiscovery: true, + entries: [], + esbuildOptions: { target: "esnext" }, + }, + esbuild: { target: "esnext" }, + plugins: [ + solid(), + { + name: "ordering-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 === "/ordering-fixture.tsx") return id; + }, + load(id) { + if (id === "/ordering-fixture.tsx") return fixture; + }, + }, + ], + resolve: { alias: { "@": resolve(root, "src") } }, + server: { host: "127.0.0.1", port: 5197, strictPort: true }, +}); +let browser; +try { + await server.listen(); + browser = await chromium.launch({ headless: true }); + + const results = []; + for (const width of [1440,390]) { + for (const old of ['old-attempt','error']) { + const page = await browser.newPage({viewport:{width,height:900}}); + await page.goto('http://127.0.0.1:5197/qualification'); + await page.waitForFunction(() => window.count?.() === 1); + await page.getByRole('button',{name:'Configuration Retry',exact:true}).click(); + await page.waitForFunction(() => window.count() === 2); + await page.evaluate(() => window.finish(1,'current-attempt')); + await page.getByText('current-attempt',{exact:true}).waitFor(); + // Held reads must not keep the attempt refresh control disabled. + const refresh = page.getByRole('button', {name:'Refresh delivery status', exact:true}); + await expect(refresh).toBeEnabled(); + await page.evaluate(() => window.finishHeld(1, 'current-held')); + await page.getByText('current-held', {exact:true}).waitFor(); + const before = await page.locator('main').innerText(); + await page.evaluate(s => { window.finish(0,s); window.finishHeld(0,s); },old); + await page.evaluate(() => new Promise(r => requestAnimationFrame(() => requestAnimationFrame(r)))); + const after = await page.locator('main').innerText(); + assert.equal(after, before, 'old completion must preserve current evidence'); + assert.match(after, /current-attempt/); + assert.match(after, /current-held/); + results.push({width,old,before,after,currentEvidencePreserved:before===after}); + await page.screenshot({path:evidence+'/'+width+'-'+old+'.png'}); + await page.close(); + } + } + for (const width of [1440, 390]) { + const page = await browser.newPage({viewport:{width,height:900}}); + await page.goto('http://127.0.0.1:5197/qualification'); + await page.waitForFunction(() => window.count?.() === 1); + await page.getByRole('button',{name:'Configuration Retry',exact:true}).click(); + await page.waitForFunction(() => window.count() === 2); + await page.evaluate(() => window.finish(0, 'old-attempt')); + const refresh = page.getByRole('button',{name:'Refresh delivery status',exact:true}); + await expect(refresh).toBeDisabled(); + await expect(page.locator('output')).toHaveText('true'); + await expect(page.getByText('old-attempt',{exact:true})).toHaveCount(0); + await page.evaluate(() => window.finish(1, 'error')); + await expect(page.getByRole('alert')).toBeVisible(); + await expect(refresh).toBeEnabled(); + await page.screenshot({path:evidence+'/'+width+'-pending-unavailable.png'}); + results.push({width,scenario:'old completion leaves newest pending; newest failure is unavailable',passed:true}); + await page.close(); + } + console.log(JSON.stringify({scope:'real Chromium, real caller/hook/card, scripted APIs; positive ordering assertions, not installed qualification',results},null,2)); + +} finally { + await browser?.close(); + await server.close(); +}