From 7a915017d4811a48f545d74d9f53e5826839da5c Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Tue, 8 Sep 2026 03:17:04 +0100 Subject: [PATCH] fix(alerts): preserve request ownership in incident error state PR #1973 introduced resource incident error state independently of the request lifecycle repair. Reconcile its state with this branch's ownership guards so superseded or disposed reads cannot report a false current failure. Reset clears errors and retry preserves cached history while clearing the failure indicator. Extend lifecycle assertions for error ownership, retry and superseded success after a current failure. Focused incident hook and panel tests pass: 3 files, 17 tests. Full merged UI browser acceptance remains separate. Change-source: pulse-maintainer --- .../v6/internal/subsystems/alerts.md | 15 +++++++ .../subsystems/frontend-primitives.md | 15 +++++++ frontend-modern/browser-verification.json | 13 +++--- .../useAlertResourceIncidentsState.test.tsx | 45 +++++++++++++++++++ .../alerts/useAlertResourceIncidentsState.ts | 7 +++ scripts/check-incident-request-ownership.mjs | 20 ++++++--- 6 files changed, 105 insertions(+), 10 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index 9bc7d0bf2..7752a9d28 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -15,6 +15,21 @@ ## Purpose +### Incident resource error-state ownership + +Resource incident errors follow the same per-resource latest-request ownership +as history and loading. Superseded or disposed reads cannot set an error; +reset clears errors and invalidates pending reads. A retry clears the current +error while retaining cached history until the current request succeeds. +A stale success cannot clear a newer failure. This reconciles the merged error +accessor with lifecycle protection, without changing notification delivery. + +Verification: resource hook lifecycle assertions cover ordering and retry; +`scripts/check-incident-request-ownership.mjs` checks the real hook and panel +in Chromium at desktop and narrow widths, including error state and retry. +Its scripted fixture is not full merged-UI or installed delivery acceptance. + + ### Retained-queue recovery feedback has no reading deadline Retry and Dismiss failures have a view-local untimed equivalent beside the diff --git a/docs/release-control/v6/internal/subsystems/frontend-primitives.md b/docs/release-control/v6/internal/subsystems/frontend-primitives.md index 2ab05ee14..d72c9f324 100644 --- a/docs/release-control/v6/internal/subsystems/frontend-primitives.md +++ b/docs/release-control/v6/internal/subsystems/frontend-primitives.md @@ -20,6 +20,21 @@ ## Purpose +### Incident resource error-state ownership + +Resource incident errors follow the same per-resource latest-request ownership +as history and loading. Superseded or disposed reads cannot set an error; +reset clears errors and invalidates pending reads. A retry clears the current +error while retaining cached history until the current request succeeds. +A stale success cannot clear a newer failure. This reconciles the merged error +accessor with lifecycle protection, without changing notification delivery. + +Verification: resource hook lifecycle assertions cover ordering and retry; +`scripts/check-incident-request-ownership.mjs` checks the real hook and panel +in Chromium at desktop and narrow widths, including error state and retry. +Its scripted fixture is not full merged-UI or installed delivery acceptance. + + ### Canonical Patrol and Assistant continuation, 2026-09-07 Patrol's Assistant context preserves unknown destructive risk and distinguishes diff --git a/frontend-modern/browser-verification.json b/frontend-modern/browser-verification.json index 95e9f413d..6b56058a5 100644 --- a/frontend-modern/browser-verification.json +++ b/frontend-modern/browser-verification.json @@ -1,13 +1,13 @@ { "version": 1, - "base_sha": "bbec8d0e9f282272080d8781d763248b3e8ea467", - "verified_at": "2026-09-08T01:55:51.776040Z", + "base_sha": "701362ac25ef28dfa253f952820eed7c5a90db12", + "verified_at": "2026-09-08T02:19:57.322199Z", "result": "passed", "changed_paths": [ "frontend-modern/src/features/alerts/useAlertResourceIncidentsState.ts" ], "content_sha256": { - "frontend-modern/src/features/alerts/useAlertResourceIncidentsState.ts": "547f128db61200fdf736327a248e9c97b41dd77eeb0a660dd20c4b78031a75b9" + "frontend-modern/src/features/alerts/useAlertResourceIncidentsState.ts": "7a4de5d23f84bffed0665edbc4c9bd7cf33120d81fc82f7e9f5d3df56cffeb45" }, "routes": [ "/qualification (isolated real hook and AlertResourceIncidentsPanel, scripted API)" @@ -27,7 +27,10 @@ "latest result survives older success/failure", "reset stays empty", "disposed state unchanged by success/failure", - "current failure reported" + "current failure reported", + "current error accessor is true on failure", + "retry clears current error and renders latest incident", + "obsolete and disposed failures do not set error accessor" ], "interactions": [ "Open row", @@ -37,7 +40,7 @@ "Resolve and reject deferred API reads in controlled order" ], "limitations": [ - "Fixture lifecycle controls; not full Alerts route, installed acceptance, or notification delivery. PR1973 error accessor absent." + "Fixture lifecycle controls and scripted API; not full merged Alerts route, installed acceptance, or notification delivery. Supplied panel does not render PR1973 error accessor; accessor verified through fixture snapshot." ], "command": "pulse-heavy-run -- node scripts/check-incident-request-ownership.mjs" } diff --git a/frontend-modern/src/features/alerts/__tests__/useAlertResourceIncidentsState.test.tsx b/frontend-modern/src/features/alerts/__tests__/useAlertResourceIncidentsState.test.tsx index 9a28d6207..3b55f05a9 100644 --- a/frontend-modern/src/features/alerts/__tests__/useAlertResourceIncidentsState.test.tsx +++ b/frontend-modern/src/features/alerts/__tests__/useAlertResourceIncidentsState.test.tsx @@ -34,6 +34,7 @@ describe('resource incident request ownership', () => { await load; expect(result.resourceIncidents()).toEqual({}); expect(result.resourceIncidentLoading()).toEqual({}); + expect(result.resourceIncidentError()).toEqual({}); expect(result.resourceIncidentPanel()).toBeNull(); }); @@ -63,10 +64,12 @@ describe('resource incident request ownership', () => { old.reject(new Error('obsolete read')); await load; const loadingAfterOldFailure = result.resourceIncidentLoading().host; + const errorAfterOldFailure = result.resourceIncidentError().host; const obsoleteNotifications = vi.mocked(notificationStore.error).mock.calls.length; current.resolve([]); await refresh; expect(loadingAfterOldFailure).toBe(true); + expect(errorAfterOldFailure).toBe(false); expect(obsoleteNotifications).toBe(0); expect(result.resourceIncidentLoading().host).toBe(false); }); @@ -79,6 +82,7 @@ describe('resource incident request ownership', () => { cleanup(); pending.reject(new Error('disposed read')); await load; + expect(result.resourceIncidentError().host).toBe(false); expect(notificationStore.error).not.toHaveBeenCalled(); }); @@ -95,6 +99,7 @@ describe('resource incident request ownership', () => { old.reject(new Error('reset-era failure')); await load; expect(result.resourceIncidentLoading().host).toBe(true); + expect(result.resourceIncidentError().host).toBe(false); expect(notificationStore.error).not.toHaveBeenCalled(); current.resolve([]); await reopened; @@ -112,6 +117,7 @@ describe('resource incident request ownership', () => { await result.refreshResourceIncidentPanel(); old.reject(new Error('obsolete failure')); await load; + expect(result.resourceIncidentError().host).toBe(false); expect(notificationStore.error).not.toHaveBeenCalled(); expect(result.resourceIncidents().host).toEqual([]); expect(result.resourceIncidentLoading().host).toBe(false); @@ -145,7 +151,46 @@ describe('resource incident request ownership', () => { await load; expect(result.resourceIncidents().first).toEqual([]); expect(result.resourceIncidentLoading()).toEqual({ first: false, second: false }); + expect(result.resourceIncidentError()).toEqual({ first: false, second: true }); }); + it('preserves the current error when a superseded success arrives', async () => { + const old = deferred(); + vi.mocked(AlertsAPI.getIncidentsForResource) + .mockReturnValueOnce(old.promise) + .mockRejectedValueOnce(new Error('current failure')); + const { result } = renderHook(useAlertResourceIncidentsState); + const load = result.openResourceIncidentPanel('host', 'Host', 'row'); + await result.refreshResourceIncidentPanel(); + old.resolve([]); + await load; + expect(result.resourceIncidentError().host).toBe(true); + expect(result.resourceIncidents()).toEqual({}); + expect(notificationStore.error).toHaveBeenCalledTimes(1); + }); + + it('clears a current error on retry without discarding cached history', async () => { + const cached = [{ id: 'retained' }] as Incidents; + const retry = deferred(); + vi.mocked(AlertsAPI.getIncidentsForResource) + .mockResolvedValueOnce(cached) + .mockRejectedValueOnce(new Error('refresh failed')) + .mockReturnValueOnce(retry.promise); + const { result } = renderHook(useAlertResourceIncidentsState); + await result.openResourceIncidentPanel('host', 'Host', 'row'); + await result.refreshResourceIncidentPanel(); + expect(result.resourceIncidentError().host).toBe(true); + expect(result.resourceIncidents().host).toEqual(cached); + const refresh = result.refreshResourceIncidentPanel(); + expect(result.resourceIncidentError().host).toBe(false); + expect(result.resourceIncidentLoading().host).toBe(true); + retry.resolve([]); + await refresh; + expect(result.resourceIncidentError().host).toBe(false); + expect(result.resourceIncidents().host).toEqual([]); + result.resetResourceIncidentsState(); + expect(result.resourceIncidentError()).toEqual({}); + }); + it('toggles the opening row and reuses loaded history for another row', async () => { vi.mocked(AlertsAPI.getIncidentsForResource).mockResolvedValue([]); const { result } = renderHook(useAlertResourceIncidentsState); diff --git a/frontend-modern/src/features/alerts/useAlertResourceIncidentsState.ts b/frontend-modern/src/features/alerts/useAlertResourceIncidentsState.ts index 90db2d125..94804fda5 100644 --- a/frontend-modern/src/features/alerts/useAlertResourceIncidentsState.ts +++ b/frontend-modern/src/features/alerts/useAlertResourceIncidentsState.ts @@ -21,6 +21,9 @@ export function useAlertResourceIncidentsState() { const [resourceIncidentLoading, setResourceIncidentLoading] = createSignal< Record >({}); + const [resourceIncidentError, setResourceIncidentError] = createSignal>( + {}, + ); const [expandedResourceIncidentIds, setExpandedResourceIncidentIds] = createSignal>( new Set(), ); @@ -44,12 +47,14 @@ export function useAlertResourceIncidentsState() { requests.set(resourceId, request); const ownsRequest = () => !disposed && requests.get(resourceId) === request; setResourceIncidentLoading((prev) => ({ ...prev, [resourceId]: true })); + setResourceIncidentError((prev) => ({ ...prev, [resourceId]: false })); try { const incidents = await AlertsAPI.getIncidentsForResource(resourceId, limit); if (!ownsRequest()) return; setResourceIncidents((prev) => ({ ...prev, [resourceId]: incidents })); } catch (error) { if (!ownsRequest()) return; + setResourceIncidentError((prev) => ({ ...prev, [resourceId]: true })); logger.error(getAlertResourceIncidentLoadFailure(), error); notificationStore.error(getAlertResourceIncidentLoadFailure()); } finally { @@ -105,6 +110,7 @@ export function useAlertResourceIncidentsState() { setResourceIncidentPanel(null); setResourceIncidents({}); setResourceIncidentLoading({}); + setResourceIncidentError({}); setExpandedResourceIncidentIds(new Set()); setResourceIncidentEventFilters(new Set(INCIDENT_EVENT_TYPES)); }; @@ -114,6 +120,7 @@ export function useAlertResourceIncidentsState() { setResourceIncidentPanel, resourceIncidents, resourceIncidentLoading, + resourceIncidentError, expandedResourceIncidentIds, resourceIncidentEventFilters, setResourceIncidentEventFilters, diff --git a/scripts/check-incident-request-ownership.mjs b/scripts/check-incident-request-ownership.mjs index 02e79df46..447f170c1 100644 --- a/scripts/check-incident-request-ownership.mjs +++ b/scripts/check-incident-request-ownership.mjs @@ -23,7 +23,7 @@ window.finish = (i, status) => status === 'error' ? pending[i].reject(new Error( window.count = () => pending.length; function Panel() { const s = useAlertResourceIncidentsState(); - window.snapshot = () => ({incidents:s.resourceIncidents(), loading:s.resourceIncidentLoading()}); + window.snapshot = () => ({incidents:s.resourceIncidents(), loading:s.resourceIncidentLoading(), error:s.resourceIncidentError()}); return
{JSON.stringify(window.snapshot())}
; } function Fixture() { @@ -83,6 +83,7 @@ try { "dispose-success", "dispose-failure", "current-failure", + "retry", ]) { const page = await browser.newPage({ viewport: { width, height: 900 } }); await page.goto("http://127.0.0.1:5198/qualification"); @@ -99,8 +100,15 @@ try { (s) => window.finish(0, s === "dispose-failure" ? "error" : "empty"), scenario, ); - } else if (scenario === "current-failure") { + } else if (scenario === "current-failure" || scenario === "retry") { await page.evaluate(() => window.finish(0, "error")); + await page.waitForFunction(() => window.snapshot().error.host === true); + if (scenario === "retry") { + await page.getByRole("button", { name: "Overlap refresh", exact: true }).click(); + await page.waitForFunction(() => window.count() === 2); + assert.equal((await page.evaluate(() => window.snapshot())).error.host, false); + await page.evaluate(() => window.finish(1, "Latest incident")); + } } else { await page .getByRole("button", { name: "Overlap refresh", exact: true }) @@ -139,16 +147,18 @@ try { const snapshot = await page.evaluate(() => window.snapshot()); assert.equal( await page.getByTestId("errors").textContent(), - scenario === "current-failure" ? "1" : "0", + ["current-failure", "retry"].includes(scenario) ? "1" : "0", ); if (scenario === "reset") { - assert.deepEqual(snapshot, { incidents: {}, loading: {} }); + assert.deepEqual(snapshot, { incidents: {}, loading: {}, error: {} }); } else if (scenario.startsWith("dispose")) { - assert.deepEqual(snapshot, { incidents: {}, loading: { host: true } }); + assert.deepEqual(snapshot, { incidents: {}, loading: { host: true }, error: { host: false } }); assert.equal(await page.locator("section").count(), 0); } else if (scenario === "current-failure") { assert.equal(snapshot.loading.host, false); + assert.equal(snapshot.error.host, true); } else { + assert.equal(snapshot.error.host, false); assert.equal(snapshot.incidents.host[0].id, "Latest incident"); assert.equal(snapshot.loading.host, false); await page.getByText("Latest incident", { exact: true }).waitFor();