From 7c48430d5a3ed0eefc677b6a5ccc50ee4bc0920a Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Fri, 2 Oct 2026 21:45:50 +0100 Subject: [PATCH 1/5] Make stored History readings inspectable by touch Real Chromium and WebKit first taps selected the latest reading instead of the tapped observation. Select and retain the actual sample across compatibility focus and mouse events, shared-chart focus transfer and matching refreshes. Reject pan, cancellation and multiple contacts without preventing native scrolling. Preserve mouse and keyboard inspection, source ownership, units and access. Change-source: pulse-maintainer --- .../subsystems/frontend-primitives.md | 23 +++ .../internal/subsystems/storage-recovery.md | 9 + .../browser-tests/history-touch.cjs | 181 ++++++++++++++++ .../browser-tests/history-touch.html | 1 + .../browser-tests/history-touch.tsx | 60 ++++++ frontend-modern/browser-verification.json | 54 +++-- .../src/components/shared/HistoryChart.tsx | 9 +- .../shared/__tests__/HistoryChart.test.tsx | 193 ++++++++++++++++++ .../components/shared/useHistoryChartState.ts | 79 ++++++- 9 files changed, 574 insertions(+), 35 deletions(-) create mode 100644 frontend-modern/browser-tests/history-touch.cjs create mode 100644 frontend-modern/browser-tests/history-touch.html create mode 100644 frontend-modern/browser-tests/history-touch.tsx diff --git a/docs/release-control/v6/internal/subsystems/frontend-primitives.md b/docs/release-control/v6/internal/subsystems/frontend-primitives.md index d945c80542..e0d9578876 100644 --- a/docs/release-control/v6/internal/subsystems/frontend-primitives.md +++ b/docs/release-control/v6/internal/subsystems/frontend-primitives.md @@ -7729,3 +7729,26 @@ actual series pixels and tooltips in desktop Chromium and phone WebKit, using the production chart and API transport with synthetic stored samples. This does not establish native collection, whole-report resolution or availability in a release. Existing request, source, units and access boundaries are unchanged. + +### Shared History touch inspection preserves native scrolling + +A primary touch tap inside a Storage History plot selects the nearest actual +observation at that horizontal position. Compatibility mouse focus must not +replace that reading with the latest sample or announce it as keyboard input. +The existing hover group and timestamp reconciliation remain shared with mouse +and keyboard inspection. Single measured-zero observations remain inspectable; +empty data and a changed selection cannot inherit an old touch. + +Movement beyond ten CSS pixels, cancellation, multiple contacts and release +outside the plot are not taps. Inspection neither prevents native touch defaults +nor captures the pointer or changes touch-action: page scrolling and pinch zoom +remain browser-owned. Blur and Escape clear inspection, and supported keyboard +keys resume the existing dated, politely announced sample navigation. + +Mounted `HistoryChart.test.tsx` regressions cover focus ordering, gesture +rejection, selection ownership, refresh, empty/zero and input coexistence. +`browser-tests/history-touch.cjs` exercises actual touchscreen taps through the +production disk/pool details and API transport in phone Chromium/WebKit, native +vertical panning in Chromium, and desktop mouse/keyboard controls. Synthetic +sample presentation is not native collector, physical device, assistive-device +or installed whole-report acceptance. diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index 4806fe41a8..ad7c1b1ee9 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -6295,3 +6295,12 @@ This changes no collector, health thresholds, history policy or server identity. `frontend-modern/src/components/Storage/__tests__/DiskList.test.tsx` exercises snapshot replacement, in-place reconciliation, missing evidence, target changes and fault/recovery using the real keyed renderer. + +Touch inspection in the existing pool and physical-disk History charts selects +an actual stored reading rather than substituting the latest sample on focus. +Page pan/cancel and multi-contact gestures do not imply a selected observation; +browser-owned scrolling remains available. This changes no source target, API, +units, collection, retention, access or recovery boundary. The production +disk/pool fixture `frontend-modern/browser-tests/history-touch.cjs` and mounted +`HistoryChart.test.tsx` regressions verify presentation/input behaviour only, +not live appliance collection or recovery success. diff --git a/frontend-modern/browser-tests/history-touch.cjs b/frontend-modern/browser-tests/history-touch.cjs new file mode 100644 index 0000000000..d00d447fb3 --- /dev/null +++ b/frontend-modern/browser-tests/history-touch.cjs @@ -0,0 +1,181 @@ +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const { chromium, webkit } = require('playwright'); + +(async () => { + const root = '/workspace/frontend-modern'; + process.chdir(root); + const artifacts = path.join(root, 'node_modules/history-touch-proof'); + fs.mkdirSync(artifacts, { recursive: true }); + const { createServer } = await import(path.join(root, 'node_modules/vite/dist/node/index.js')); + const server = await createServer({ + root, configFile: path.join(root, 'vite.config.ts'), + cacheDir: fs.mkdtempSync(path.join(artifacts, 'vite-')), + server: { host: '127.0.0.1', port: 5227, strictPort: true }, + }); + const parent = false; + let browser; + const observations = []; + const timestamp = Date.parse('2026-10-02T10:00:00Z'); + const point = (value, index = 0) => ({ timestamp: timestamp + index * 60000, value, min: value, max: value }); + const playwright = require('playwright/package.json').version; + const expectedPlaywright = JSON.parse(fs.readFileSync('/workspace/tests/integration/package-lock.json', 'utf8')).packages['node_modules/@playwright/test'].version; + assert.equal(playwright, expectedPlaywright); + try { + await server.listen(); + const configs = [ + ['chromium-desktop', chromium, false, false], + ['chromium-phone', chromium, true, false], + ['webkit-phone', webkit, true, true], + ]; + for (const [name, engine, touch, dark] of configs) { + if (parent && !touch) continue; + browser = await engine.launch(engine === chromium ? { headless: true, channel: 'chromium', args: ['--no-sandbox'] } : { headless: true }); + const page = await browser.newPage({ + viewport: touch ? { width: 390, height: 844 } : { width: 1365, height: 900 }, + hasTouch: touch, isMobile: touch, timezoneId: 'Europe/London', + }); + page.setDefaultTimeout(15000); + const errors = []; + const requests = []; + page.on('pageerror', error => errors.push(error.message)); + let samples = [point(10), point(20, 1), point(30, 2)]; + await page.route('**/*', route => { + const url = new URL(route.request().url()); + if (url.origin !== 'http://127.0.0.1:5227') return route.abort(); + if (!url.pathname.startsWith('/api/')) return route.continue(); + if (url.pathname === '/api/metrics-store/history') { + requests.push(Object.fromEntries(url.searchParams)); + return route.fulfill({ json: { points: samples, source: 'store' } }); + } + return route.fulfill({ json: url.pathname === '/api/license/runtime-capabilities' + ? { capabilities: [], limits: [], max_history_days: 7, hosted_mode: false, runtime: { build: 'community' }, blocked_capabilities: [] } + : { data: [], enabled: false } }); + }); + await page.goto('http://127.0.0.1:5227/browser-tests/history-touch.html'); + if (dark) await page.evaluate(() => document.documentElement.classList.add('dark')); + const disk = page.getByRole('region', { name: 'Disk fixture' }); + const pool = page.getByRole('region', { name: 'Pool fixture' }); + await disk.getByRole('tab', { name: 'History', exact: true }).click(); + const busy = disk.getByRole('img', { name: 'Busy chart', exact: true }); + const description = chart => chart.evaluate(el => document.getElementById(el.getAttribute('aria-describedby')).textContent); + await page.waitForFunction(() => [...document.querySelectorAll('canvas')].some(el => el.getAttribute('aria-label') === 'Busy chart' && document.getElementById(el.getAttribute('aria-describedby')).textContent.includes('3 data points'))); + const tooltip = chart => chart.locator('..').locator('[data-history-chart-tooltip]'); + const live = chart => chart.locator('..').locator('[aria-live="polite"]'); + async function inspect(chart, fraction) { + await chart.scrollIntoViewIfNeeded(); + const box = await chart.boundingBox(); + // Percent labels reserve 40px at left; the time label reserves less than + // 25px at right. These interior positions are nearest to each real sample. + const x = box.x + 45 + (box.width - 70) * fraction; + const y = box.y + box.height / 2; + if (touch) await page.touchscreen.tap(x, y); + else await page.mouse.move(x, y); + await page.waitForTimeout(80); + return (await tooltip(chart).count()) ? await tooltip(chart).textContent() : null; + } + async function record(phase, chart, text) { + const dimensions = await page.evaluate(() => ({ scroll: document.documentElement.scrollWidth, width: innerWidth })); + assert.ok(dimensions.scroll <= dimensions.width + 1, JSON.stringify(dimensions)); + observations.push({ name, browser: browser.version(), touch, dark, phase, text, description: await description(chart), dimensions }); + await page.screenshot({ path: path.join(artifacts, `${parent ? 'parent-' : ''}${name}-${phase}.png`) }); + } + if (parent) { + for (const [phase, fraction, expected] of [['first', 0.04, '10.0%'], ['middle', 0.5, '20.0%']]) { + const text = await inspect(busy, fraction); + if (phase === 'first') assert.ok(!text?.includes(expected), `predecessor unexpectedly inspected the first tap: ${text}`); + await record(phase, busy, text); + } + } else { + for (const [phase, fraction, expected] of [['first', 0.04, '10.0%'], ['middle', 0.5, '20.0%'], ['last', 0.96, '30.0%']]) { + const text = await inspect(busy, fraction); + assert.ok(text?.includes(expected), `${name} ${phase}: ${text}`); + assert.equal(await live(busy).textContent(), '', 'pointer inspection must not speak keyboard announcements'); + assert.equal(await disk.locator('[data-history-chart-tooltip]').count(), 3, 'live I/O group shares inspection'); + await record(phase, busy, text); + } + if (touch) { + const read = disk.getByRole('img', { name: 'Read chart', exact: true }); + assert.ok((await inspect(read, 0.5))?.includes('20.0 B/s')); + assert.ok((await tooltip(busy).textContent()).includes('20.0%'), 'a peer blur must not clear the new group tap'); + assert.equal(await live(read).textContent(), ''); + await inspect(busy, 0.5); + // A hybrid device may switch back to a real mouse after a tap. + await busy.scrollIntoViewIfNeeded(); + const box = await busy.boundingBox(); + await page.mouse.move(box.x + box.width / 2, box.y + box.height / 2); + assert.ok((await tooltip(busy).textContent()).includes('20.0%')); + await page.mouse.move(0, 0); + assert.equal(await tooltip(busy).count(), 0); + } + await busy.focus(); + for (const [key, expected] of [['Home', '10.0%'], ['ArrowRight', '20.0%'], ['End', '30.0%']]) { + await page.keyboard.press(key); + assert.ok((await live(busy).textContent()).includes(expected)); + } + await page.keyboard.press('Escape'); + assert.equal(await tooltip(busy).count(), 0); + await page.getByRole('button', { name: 'After charts' }).focus(); + assert.equal(await live(busy).textContent(), ''); + + if (touch && engine === chromium) { + await busy.scrollIntoViewIfNeeded(); + await inspect(busy, 0.5); + const box = await busy.boundingBox(); + const session = await page.context().newCDPSession(page); + await busy.evaluate(el => { + el.dataset.cancelCount = '0'; + el.addEventListener('pointercancel', () => el.dataset.cancelCount = String(Number(el.dataset.cancelCount) + 1)); + }); + const scrollMetrics = await page.evaluate(() => ({ before: scrollY, maximum: document.documentElement.scrollHeight - innerHeight })); + assert.ok(scrollMetrics.maximum > scrollMetrics.before + 200, 'the fixture must have room to scroll'); + const before = scrollMetrics.before; + const x = box.x + box.width / 2, y = box.y + box.height / 2; + await session.send('Input.dispatchTouchEvent', { type: 'touchStart', touchPoints: [{ x, y, id: 1 }] }); + for (let step = 1; step <= 5; step++) { + await session.send('Input.dispatchTouchEvent', { type: 'touchMove', touchPoints: [{ x, y: y - step * 20, id: 1 }] }); + await page.waitForTimeout(50); + } + await session.send('Input.dispatchTouchEvent', { type: 'touchEnd', touchPoints: [] }); + await page.waitForTimeout(300); + const after = await page.evaluate(() => scrollY); + fs.writeFileSync(path.join(artifacts, 'pan-control.json'), JSON.stringify({before, after, pointerCancels: await busy.getAttribute('data-cancel-count'), touchAction: await busy.evaluate(el => getComputedStyle(el).touchAction)},null,2)); + assert.ok(after > before + 40, `native pan must scroll: ${before} -> ${after}`); + assert.ok(Number(await busy.getAttribute('data-cancel-count')) > 0); + assert.equal(await tooltip(busy).count(), 0, 'native scrolling must not select a reading'); + observations.push({ name, phase: 'native-vertical-pan', before, after, maximum: scrollMetrics.maximum, pointerCancels: Number(await busy.getAttribute('data-cancel-count')), touchAction: await busy.evaluate(el => getComputedStyle(el).touchAction) }); + await session.detach(); + } + + await pool.getByRole('tab', { name: 'History', exact: true }).click(); + const usage = pool.getByRole('img', { name: 'Usage chart', exact: true }); + await usage.waitFor(); + await page.waitForFunction(() => [...document.querySelectorAll('canvas')].some(el => el.getAttribute('aria-label') === 'Usage chart' && document.getElementById(el.getAttribute('aria-describedby')).textContent.includes('3 data points'))); + assert.ok((await inspect(usage, 0.04))?.includes('10.0%')); + await record('pool-first', usage, await tooltip(usage).textContent()); + assert.equal(await pool.getByLabel('Capacity history range').inputValue(), '7d'); + samples = [point(0)]; + await pool.getByLabel('Capacity history range').selectOption('24h'); + await page.waitForFunction(() => [...document.querySelectorAll('canvas')].some(el => el.getAttribute('aria-label') === 'Usage chart' && document.getElementById(el.getAttribute('aria-describedby')).textContent.includes('1 data point'))); + assert.ok((await inspect(usage, 0.5))?.includes('0.0%')); + await record('pool-zero-singleton', usage, await tooltip(usage).textContent()); + samples = []; + await pool.getByLabel('Capacity history range').selectOption('7d'); + await pool.getByText('No history samples in this time range.', { exact: true }).waitFor(); + assert.equal(await inspect(usage, 0.5), null); + assert.equal(await live(usage).textContent(), ''); + await record('pool-empty', usage, null); + assert.ok(requests.some(request => request.resourceType === 'disk' && request.resourceId === 'fixture-disk')); + assert.ok(requests.some(request => request.resourceType === 'storage' && request.resourceId === 'fixture-pool')); + } + assert.deepEqual(errors, []); + await browser.close(); browser = null; + } + fs.writeFileSync(path.join(artifacts, parent ? 'parent.json' : 'result.json'), JSON.stringify({ playwright, expectedPlaywright, parent, observations }, null, 2)); + console.log(JSON.stringify({ result: 'passed', parent, observations: observations.length })); + } finally { + if (browser) await browser.close(); + await server.close(); + } +})().catch(error => { console.error(error); process.exitCode = 1; }); diff --git a/frontend-modern/browser-tests/history-touch.html b/frontend-modern/browser-tests/history-touch.html new file mode 100644 index 0000000000..ec56435f82 --- /dev/null +++ b/frontend-modern/browser-tests/history-touch.html @@ -0,0 +1 @@ +Storage History touch verification
diff --git a/frontend-modern/browser-tests/history-touch.tsx b/frontend-modern/browser-tests/history-touch.tsx new file mode 100644 index 0000000000..c5eef346b3 --- /dev/null +++ b/frontend-modern/browser-tests/history-touch.tsx @@ -0,0 +1,60 @@ +import { render } from 'solid-js/web'; +import { DiskDetail } from '../src/components/Storage/DiskDetail'; +import { StoragePoolDetail } from '../src/components/Storage/StoragePoolDetail'; +import type { StorageRecord } from '../src/features/storageBackups/models'; +import type { Resource } from '../src/types/resource'; +import '../src/index.css'; + +const disk = { + id: 'fixture-disk', + type: 'physical_disk', + name: 'Archive HDD', + displayName: 'Archive HDD', + platformType: 'proxmox-pve', + platformId: 'fixture', + sourceType: 'api', + status: 'online', + lastSeen: Date.now(), + identity: { hostname: 'storage-host' }, + metricsTarget: { resourceType: 'disk', resourceId: 'fixture-disk' }, + physicalDisk: { devPath: '/dev/sda', model: 'Archive HDD', diskType: 'hdd', temperature: 42 }, +} as Resource; + +const pool: StorageRecord = { + id: 'fixture-pool', + name: 'Archive', + category: 'datastore', + health: 'unknown', + location: { label: 'archive-host', scope: 'host' }, + source: { + platform: 'proxmox-pbs', + family: 'onprem', + origin: 'resource', + adapterId: 'resource-storage', + }, + capacity: { totalBytes: 1024, usedBytes: 512, freeBytes: 512, usagePercent: 50 }, + capabilities: ['capacity'], + observedAt: Date.now(), +}; + +render( + () => ( +
+

Storage History touch verification

+

Synthetic stored samples in production disk and pool details.

+
+ +
+
+ + + + +
+
+ +
+ ), + document.getElementById('root')!, +); diff --git a/frontend-modern/browser-verification.json b/frontend-modern/browser-verification.json index d836db5f31..395e434f7a 100644 --- a/frontend-modern/browser-verification.json +++ b/frontend-modern/browser-verification.json @@ -1,18 +1,18 @@ { "version": 1, - "base_sha": "ad7f7a1063d45e26f77faf35bacbca6877f41cac", - "verified_at": "2026-10-02T18:37:50.309899Z", + "base_sha": "e5d0843851a3837a90d71afaced3a009c0606bbf", + "verified_at": "2026-10-02T20:44:41.220410Z", "result": "passed", "changed_paths": [ - "frontend-modern/src/components/shared/historyChartModel.ts", + "frontend-modern/src/components/shared/HistoryChart.tsx", "frontend-modern/src/components/shared/useHistoryChartState.ts" ], "content_sha256": { - "frontend-modern/src/components/shared/historyChartModel.ts": "c96437081022b073dca0c730a8617954987f712ee84e058c6078ae3cad05f162", - "frontend-modern/src/components/shared/useHistoryChartState.ts": "38af5f416fb95874e708b08d66fdbcf27b85660840e0096cb1dd24d9bc744991" + "frontend-modern/src/components/shared/HistoryChart.tsx": "74fb7e6b466e1c2b2d2efb13e0a124d6c301896a7c9423432e903908cc9ee25b", + "frontend-modern/src/components/shared/useHistoryChartState.ts": "d6e8317019c8dc616bd9a4d3c6c71cb5329297ca0341e6ba28a6cc4c84db36b0" }, "routes": [ - "/browser-tests/history-sparse.html" + "/browser-tests/history-touch.html" ], "viewports": [ { @@ -25,37 +25,33 @@ } ], "states": [ - "Single stored 42% before inspection, pointer inspection, measured zero, multiple observations, successful empty response, restored singleton", - "Desktop Chromium light and phone WebKit dark-class presentation; 12 observations, no page errors or horizontal overflow" + "Production disk/pool details with stored multi-point, measured-zero singleton and empty history; 19 observed presentations across desktop Chromium and phone Chromium/WebKit", + "Actual touchscreen first/middle/last taps, shared I/O group focus transfer, keyboard navigation/exit and real mouse coexistence; phone WebKit dark-class presentation" ], "interactions": [ - "Pointer at three plot positions resolves to the real singleton timestamp; no mouse live announcements", - "Tab, Home/End, Left/Right, Escape and focus exit; matching API refreshes across sparse, populated and empty results", - "Both actual predecessor engines reproduce zero series pixels and missing pointer tooltip. Parent phone and final phone singleton/tooltip/zero/empty plus final desktop multi screenshots inspected." + "Native touchscreen taps select the actual reading at that position instead of the latest sample; touch compatibility focus/mouse movement/leave cannot overwrite it", + "Primary contact cancellation, non-tap gesture rejection and native Chromium vertical pan with room-to-scroll precondition; no prevented touch default, pointer capture or touch-action change", + "Desktop/phone keyboard Home/Right/End/Escape and focus exit, pointer announcements remain quiet, range replacement, zero/empty responses and no horizontal overflow", + "Predecessor native Chromium/WebKit first tap near 10% instead displays 30%; final first/middle/last taps display 10/20/30%. Inspected predecessor and final phone, zero/empty and desktop screenshots" ], - "command": "pulse-worker-browser frontend-modern/browser-tests/history-sparse.cjs", + "command": "pulse-worker-browser frontend-modern/browser-tests/history-touch.cjs", "browser_versions": { "playwright": "1.56.1", "chromium": "141.0.7390.37", "webkit": "26.0" }, "artifacts": [ - "/var/lib/pulse-maintainer/worker-outputs/web-product-i9rj2yde/browser/result.json", - "/var/lib/pulse-maintainer/worker-outputs/web-product-i9rj2yde/browser/parent.json", - "/var/lib/pulse-maintainer/worker-outputs/web-product-i9rj2yde/browser/parent-chromium-single.png", - "/var/lib/pulse-maintainer/worker-outputs/web-product-i9rj2yde/browser/parent-webkit-single.png", - "/var/lib/pulse-maintainer/worker-outputs/web-product-i9rj2yde/browser/chromium-single.png", - "/var/lib/pulse-maintainer/worker-outputs/web-product-i9rj2yde/browser/chromium-pointer.png", - "/var/lib/pulse-maintainer/worker-outputs/web-product-i9rj2yde/browser/chromium-zero.png", - "/var/lib/pulse-maintainer/worker-outputs/web-product-i9rj2yde/browser/chromium-multiple.png", - "/var/lib/pulse-maintainer/worker-outputs/web-product-i9rj2yde/browser/chromium-empty.png", - "/var/lib/pulse-maintainer/worker-outputs/web-product-i9rj2yde/browser/chromium-recovered-single.png", - "/var/lib/pulse-maintainer/worker-outputs/web-product-i9rj2yde/browser/webkit-single.png", - "/var/lib/pulse-maintainer/worker-outputs/web-product-i9rj2yde/browser/webkit-pointer.png", - "/var/lib/pulse-maintainer/worker-outputs/web-product-i9rj2yde/browser/webkit-zero.png", - "/var/lib/pulse-maintainer/worker-outputs/web-product-i9rj2yde/browser/webkit-multiple.png", - "/var/lib/pulse-maintainer/worker-outputs/web-product-i9rj2yde/browser/webkit-empty.png", - "/var/lib/pulse-maintainer/worker-outputs/web-product-i9rj2yde/browser/webkit-recovered-single.png" + "browser/result.json", + "browser/chromium-phone-first.png", + "browser/webkit-phone-first.png", + "browser/webkit-phone-middle.png", + "browser/webkit-phone-pool-first.png", + "browser/webkit-phone-pool-zero-singleton.png", + "browser/webkit-phone-pool-empty.png", + "browser/chromium-desktop-middle.png", + "browser/parent.json", + "browser/parent-chromium-phone-first.png", + "browser/parent-webkit-phone-first.png" ], - "limitations": "Synthetic samples, not native PBS/TrueNAS collection, assistive-device testing or whole-application theme-transition acceptance." + "limitations": "Synthetic stored APIs, not native PBS/TrueNAS collection, physical phones or assistive-device speech. Native pan is Chromium-only; no physical/WebKit pan, pinch-zoom or whole-app theme-transition claim. Initial fixture and pan-driver failures are retained separately." } diff --git a/frontend-modern/src/components/shared/HistoryChart.tsx b/frontend-modern/src/components/shared/HistoryChart.tsx index ac36f7f4e7..47841fccb2 100644 --- a/frontend-modern/src/components/shared/HistoryChart.tsx +++ b/frontend-modern/src/components/shared/HistoryChart.tsx @@ -78,10 +78,15 @@ export const HistoryChart: Component = (props) => { aria-describedby={descriptionId} onMouseMove={chart.handleMouseMove} onMouseLeave={chart.handleMouseLeave} + onPointerDown={chart.handlePointerDown} + onPointerMove={chart.handlePointerMove} + onPointerUp={chart.handlePointerUp} + onPointerCancel={chart.handlePointerCancel} />

- {refreshStatus()} {accessibleDescription()} Use Left and Right arrow keys to inspect - readings, Home and End for the first and last reading, and Escape to clear inspection. + {refreshStatus()} {accessibleDescription()} Tap the chart to inspect a reading, or use + Left and Right arrow keys, Home and End for the first and last reading, and Escape to + clear inspection.

{chart.keyboardInspecting() && chart.hoveredPoint() diff --git a/frontend-modern/src/components/shared/__tests__/HistoryChart.test.tsx b/frontend-modern/src/components/shared/__tests__/HistoryChart.test.tsx index d2eb0a84b9..8729c8a2ba 100644 --- a/frontend-modern/src/components/shared/__tests__/HistoryChart.test.tsx +++ b/frontend-modern/src/components/shared/__tests__/HistoryChart.test.tsx @@ -15,6 +15,63 @@ import { HISTORY_CHART_RANGES, } from '@/components/shared/historyChartModel'; +function touchPointer(canvas: Element, type: string, fields: Partial = {}) { + const event = Object.assign(new Event(type, { bubbles: true, cancelable: true }), { + pointerId: 1, + pointerType: 'touch', + isPrimary: true, + clientX: 60, + clientY: 60, + ...fields, + }); + fireEvent(canvas, event); + // Inspection must never consume native page panning or pinch zoom. + expect(event.defaultPrevented).toBe(false); +} + +function mountTouchHistory() { + const [target, setTarget] = createSignal('disk-a'); + const [data, setData] = createSignal( + [10, 20, 30].map((value, index) => ({ + timestamp: (index + 1) * 1000, + value, + min: value, + max: value, + })), + ); + const view = render(() => ( + + )); + const canvas = view.container.querySelector('canvas')!; + vi.spyOn(canvas, 'getBoundingClientRect').mockReturnValue({ + x: 0, + y: 0, + left: 0, + top: 0, + right: 400, + bottom: 200, + width: 400, + height: 200, + toJSON: () => ({}), + }); + return { + ...view, + canvas, + setTarget, + data, + setData, + tooltip: () => view.container.querySelector('[data-history-chart-tooltip]'), + announcement: view.container.querySelector('[aria-live="polite"]')!, + }; +} + if (typeof globalThis.ResizeObserver === 'undefined') { globalThis.ResizeObserver = class ResizeObserver { observe() {} @@ -181,6 +238,142 @@ describe('HistoryChart', () => { expect(announcement.textContent).toBe(''); }); + it('inspects the tapped sample without compatibility focus selecting the latest reading', () => { + const view = mountTouchHistory(); + touchPointer(view.canvas, 'pointerdown'); + touchPointer(view.canvas, 'pointerup'); + fireEvent.focus(view.canvas); + expect(view.tooltip()).toHaveTextContent('10.0%'); + expect(view.announcement.textContent).toBe(''); + fireEvent.mouseMove(view.canvas, { clientX: 370 }); + fireEvent.mouseLeave(view.canvas); + expect(view.tooltip()).toHaveTextContent('10.0%'); + touchPointer(view.canvas, 'pointerdown', { clientX: 200 }); + touchPointer(view.canvas, 'pointerup', { clientX: 200 }); + expect(view.tooltip()).toHaveTextContent('20.0%'); + fireEvent.keyDown(view.canvas, { key: 'ArrowRight' }); + expect(view.announcement).toHaveTextContent('30.0%'); + fireEvent.keyDown(view.canvas, { key: 'Escape' }); + expect(view.tooltip()).toBeNull(); + fireEvent.blur(view.canvas); + fireEvent.focus(view.canvas); + expect(view.announcement).toHaveTextContent('30.0%'); + }); + + it.each(['moved', 'release-displaced', 'cancelled', 'second-finger', 'different-pointer'])( + 'does not turn a %s touch into inspection', + (gesture) => { + const view = mountTouchHistory(); + touchPointer(view.canvas, 'pointerdown'); + if (gesture === 'moved') { + touchPointer(view.canvas, 'pointermove', { clientY: 90 }); + // Returning to the origin is still a drag, not a new tap. + touchPointer(view.canvas, 'pointermove'); + } else if (gesture === 'cancelled') { + touchPointer(view.canvas, 'pointercancel'); + } else if (gesture === 'second-finger') { + touchPointer(view.canvas, 'pointerdown', { pointerId: 2, isPrimary: false }); + } + touchPointer(view.canvas, 'pointerup', { + ...(gesture === 'release-displaced' ? { clientY: 90 } : {}), + ...(gesture === 'different-pointer' ? { pointerId: 2 } : {}), + }); + fireEvent.focus(view.canvas); + expect(view.tooltip()).toBeNull(); + expect(view.announcement.textContent).toBe(''); + }, + ); + + it.each([{ clientX: 0 }, { clientX: 400 }, { clientY: -1 }, { clientY: 201 }])( + 'does not inspect a tap outside the plot: %j', + (position) => { + const view = mountTouchHistory(); + touchPointer(view.canvas, 'pointerdown', position); + touchPointer(view.canvas, 'pointerup', position); + expect(view.tooltip()).toBeNull(); + }, + ); + + it('invalidates an in-flight touch when the resource changes', () => { + const view = mountTouchHistory(); + touchPointer(view.canvas, 'pointerdown'); + view.setTarget('disk-b'); + touchPointer(view.canvas, 'pointerup'); + fireEvent.focus(view.canvas); + expect(view.tooltip()).toBeNull(); + expect(view.announcement.textContent).toBe(''); + touchPointer(view.canvas, 'pointerdown', { clientX: 200 }); + touchPointer(view.canvas, 'pointerup', { clientX: 200 }); + expect(view.tooltip()).toHaveTextContent('20.0%'); + }); + + it('keeps the tapped time on refresh and distinguishes a lone zero from empty history', () => { + const view = mountTouchHistory(); + touchPointer(view.canvas, 'pointerdown'); + touchPointer(view.canvas, 'pointerup'); + view.setData(view.data().map((point) => ({ ...point, value: point.value + 1 }))); + expect(view.tooltip()).toHaveTextContent('11.0%'); + view.setData([{ timestamp: 1000, value: 0, min: 0, max: 0 }]); + touchPointer(view.canvas, 'pointerdown', { clientX: 200 }); + touchPointer(view.canvas, 'pointerup', { clientX: 200 }); + expect(view.tooltip()).toHaveTextContent('0.0%'); + view.setData([]); + touchPointer(view.canvas, 'pointerdown'); + touchPointer(view.canvas, 'pointerup'); + fireEvent.focus(view.canvas); + expect(view.tooltip()).toBeNull(); + expect(view.announcement.textContent).toBe(''); + }); + + it('leaves mouse hover available after touch inspection', () => { + const view = mountTouchHistory(); + touchPointer(view.canvas, 'pointerdown'); + touchPointer(view.canvas, 'pointerup'); + touchPointer(view.canvas, 'pointermove', { pointerType: 'mouse', clientX: 200 }); + fireEvent.mouseMove(view.canvas, { clientX: 200 }); + expect(view.tooltip()).toHaveTextContent('20.0%'); + fireEvent.mouseLeave(view.canvas); + expect(view.tooltip()).toBeNull(); + }); + + it('keeps a new group chart tap when the previously focused chart blurs', () => { + const points = [10, 20, 30].map((value, index) => ({ + timestamp: (index + 1) * 1000, + value, + min: value, + max: value, + })); + const { container } = render(() => ( + + + + + )); + const [previous, next] = container.querySelectorAll('canvas'); + vi.spyOn(next, 'getBoundingClientRect').mockReturnValue({ + x: 0, + y: 0, + left: 0, + top: 0, + right: 400, + bottom: 200, + width: 400, + height: 200, + toJSON: () => ({}), + }); + fireEvent.focus(previous); + touchPointer(next, 'pointerdown'); + touchPointer(next, 'pointerup'); + fireEvent.blur(previous); + fireEvent.focus(next); + const tooltips = container.querySelectorAll('[data-history-chart-tooltip]'); + expect(tooltips).toHaveLength(2); + for (const tooltip of tooltips) expect(tooltip).toHaveTextContent('10.0%'); + for (const live of container.querySelectorAll('[aria-live="polite"]')) { + expect(live.textContent).toBe(''); + } + }); + it('renders the default history label', () => { render(() => ); diff --git a/frontend-modern/src/components/shared/useHistoryChartState.ts b/frontend-modern/src/components/shared/useHistoryChartState.ts index 93d36881d5..2ccfa69c54 100644 --- a/frontend-modern/src/components/shared/useHistoryChartState.ts +++ b/frontend-modern/src/components/shared/useHistoryChartState.ts @@ -48,6 +48,9 @@ export function useHistoryChartState( const chartHeight = createMemo(() => props.height || 200); let chartLeftInset = HISTORY_CHART_MIN_LEFT_INSET; let chartRightInset = 0; + let touchFocus = false; + let touchTimestamp: number | null = null; + let pendingTouch: { pointerId: number; x: number; y: number; moved: boolean } | undefined; const hoveredTimestamp = hoverGroup?.hoveredTimestamp ?? localHoveredTimestamp; const setHoveredTimestamp = hoverGroup?.setHoveredTimestamp ?? setLocalHoveredTimestamp; @@ -132,6 +135,8 @@ export function useHistoryChartState( suppliedData !== undefined, ]); if (selection !== previousSelection) { + pendingTouch = undefined; + touchTimestamp = null; setHoveredPoint(null); setHoveredTimestamp(null); } @@ -376,12 +381,21 @@ export function useHistoryChartState( }); const handleFocus = () => { + // Touch's compatibility mouse events can focus the canvas after pointerup. + // That focus must not replace the tapped reading with the latest sample. + if (touchFocus) { + setHoveredTimestamp(touchTimestamp); + return; + } setKeyboardInspecting(true); const points = data(); setHoveredTimestamp(points.length ? points[points.length - 1].timestamp : null); }; const handleBlur = () => { + touchFocus = false; + touchTimestamp = null; + pendingTouch = undefined; setKeyboardInspecting(false); setHoveredTimestamp(null); }; @@ -389,12 +403,18 @@ export function useHistoryChartState( const handleKeyDown = (event: KeyboardEvent) => { if (event.altKey || event.ctrlKey || event.metaKey) return; if (event.key === 'Escape') { + touchFocus = false; + touchTimestamp = null; + pendingTouch = undefined; setHoveredTimestamp(null); return; } const points = data(); if (!points.length || !['ArrowLeft', 'ArrowRight', 'Home', 'End'].includes(event.key)) return; event.preventDefault(); + touchFocus = false; + touchTimestamp = null; + pendingTouch = undefined; setKeyboardInspecting(true); const timestamp = hoveredTimestamp(); const index = @@ -410,14 +430,14 @@ export function useHistoryChartState( setHoveredTimestamp(points[next].timestamp); }; - const handleMouseMove = (event: MouseEvent) => { + const inspectAtClientX = (clientX: number, snapToSample = false) => { const canvas = refs.getCanvas(); const points = data(); if (!canvas || points.length === 0) return; setKeyboardInspecting(false); const rect = canvas.getBoundingClientRect(); - const x = event.clientX - rect.left; + const x = clientX - rect.left; const width = rect.width; if (x < chartLeftInset || x > width - chartRightInset) { setHoveredTimestamp(null); @@ -434,11 +454,58 @@ export function useHistoryChartState( leftInset: chartLeftInset, rightInset: chartRightInset, }); - setHoveredTimestamp(geometry.getTimestamp(x)); + const timestamp = geometry.getTimestamp(x); + const inspectedTimestamp = snapToSample + ? findHistoryChartClosestPoint(points, timestamp).timestamp + : timestamp; + setHoveredTimestamp(inspectedTimestamp); + return inspectedTimestamp; + }; + + const handleMouseMove = (event: MouseEvent) => { + if (!touchFocus) inspectAtClientX(event.clientX); + }; + + const handlePointerDown = (event: PointerEvent) => { + if (event.pointerType === 'mouse') touchFocus = false; + if (event.pointerType !== 'touch') return; + touchFocus = true; + touchTimestamp = null; + setKeyboardInspecting(false); + setHoveredTimestamp(null); + pendingTouch = event.isPrimary + ? { pointerId: event.pointerId, x: event.clientX, y: event.clientY, moved: false } + : undefined; + }; + + const handlePointerMove = (event: PointerEvent) => { + if (event.pointerType === 'mouse') touchFocus = false; + if (event.pointerType !== 'touch' || pendingTouch?.pointerId !== event.pointerId) return; + if (Math.hypot(event.clientX - pendingTouch.x, event.clientY - pendingTouch.y) > 10) { + pendingTouch.moved = true; + } + }; + + const handlePointerUp = (event: PointerEvent) => { + if (event.pointerType !== 'touch') return; + const touch = pendingTouch; + pendingTouch = undefined; + if (!touch || !event.isPrimary || touch.pointerId !== event.pointerId || touch.moved) return; + if (Math.hypot(event.clientX - touch.x, event.clientY - touch.y) > 10) return; + const rect = refs.getCanvas()?.getBoundingClientRect(); + if (!rect || event.clientY < rect.top || event.clientY > rect.bottom) return; + touchTimestamp = inspectAtClientX(event.clientX, true) ?? null; + }; + + const handlePointerCancel = (event: PointerEvent) => { + if (event.pointerType !== 'touch') return; + pendingTouch = undefined; + touchTimestamp = null; + setHoveredTimestamp(null); }; const handleMouseLeave = () => { - if (keyboardInspecting()) return; + if (keyboardInspecting() || touchFocus) return; setHoveredTimestamp(null); }; @@ -454,6 +521,10 @@ export function useHistoryChartState( keyboardInspecting, handleMouseLeave, handleMouseMove, + handlePointerDown, + handlePointerMove, + handlePointerUp, + handlePointerCancel, chartHeight, chartWidth, hoveredPoint, From b8cc3b44f8b7e1bd927b0368b10d59f37ede2187 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Fri, 2 Oct 2026 22:24:02 +0100 Subject: [PATCH 2/5] Skip empty TrueNAS app stats and explain session disposal Avoid a needless subscription when Apps has no inventory, preserve second-attempt stream cleanup and bounded log-tail completion, and record only classified local socket-close causes without private wire data. This repairs the empty-inventory path in #2396; the exact rejection envelope and the cause of #2400 remain unconfirmed. Change-source: pulse-maintainer --- .../v6/internal/subsystems/monitoring.md | 41 +++ internal/truenas/client.go | 20 +- internal/truenas/transport.go | 98 ++++++- internal/truenas/transport_test.go | 276 +++++++++++++++++- 4 files changed, 415 insertions(+), 20 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index a8a2e2ebed..c7ba9d2954 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -196,6 +196,47 @@ server, opening-context cancellation, concurrent controls/RPCs and sender dispos cycles, completion timestamps and unchanged failure backoff. These are synthetic runtime proofs, not native firmware timeout or reporter-resolution evidence. +### TrueNAS empty app inventories and session-disposal evidence + +An empty or null `app.query`/legacy `/app` result remains a successful empty +app inventory. It must not request `app.stats`: there is no workload to enrich, +and a stopped Apps service must not consume the operation budget or dispose of +an otherwise usable monitoring session just to enrich no rows. Nonempty app +inventories retain best-effort stats enrichment. This removes a needless +subscription path reported in #2396; it does not establish the exact +`notify_unsubscribed` envelope or resolve rejection with nonempty inventories. +Unrecognised stream notifications remain bounded by the existing operation +timeout, not classified as a known subscription's rejection without evidence. + +The one permitted stream transport retry uses the same disposal rules as the +initial exchange. Malformed events, empty subscription IDs and failed +unsubscribe cleanup discard the second session and report disconnected status; +normal app-log idle completion returns the collected tail and discards its +timed-out socket rather than surfacing an internal consumed-session sentinel. +No extra retry, action replay or REST downgrade is introduced. + +Each authenticated socket's first local disposal records an INFO event with +`component=truenas_rpc`, `action=close_session`, a fixed `reason`, authenticated +`session_age`, and optional request-method/error-category classifications. Causes +distinguish client closure, stream completion/error, transport error and failed +keepalive writes. Error categories distinguish timeout, cancellation, method, +transport and protocol errors. No error text, params, subscription ID, endpoint, +username or credential is logged. Keepalive failure may precede a subsequent +poll's disposal, so that first cause must be preserved and logged only once. +These are local close-path observations, not appliance/proxy diagnoses or +evidence that #2400's minute-by-minute reauthentication has been resolved. There +is still no fixed session-age or per-poll closure policy. + +Verification in `internal/truenas/transport_test.go`: +`TestJSONRPCEmptyAppsDoNotSubscribeToStats`, +`TestLegacyRESTEmptyAppsDoNotNegotiateStats`, the snapshot inventory contract, +`TestJSONRPCStreamRetryDiscardsUnusableSecondSession`, +`TestJSONRPCStreamRetryReturnsCompletedLogTail`, `TestRPCSessionCloseDetails`, +`TestRPCSessionCloseLogIsBoundedAndSecretFree` and +`TestRPCSessionCloseLogKeepsFirstKeepaliveCause`. These controlled runtime tests +preserve inventory, retry limits, fresh-session recovery and log confidentiality; +native SCALE behaviour, published delivery and field acceptance remain separate. + ### TrueNAS reporting summaries preserve raw History — issue #2346 diff --git a/internal/truenas/client.go b/internal/truenas/client.go index 6ad3e11046..ada44b00fb 100644 --- a/internal/truenas/client.go +++ b/internal/truenas/client.go @@ -260,7 +260,7 @@ func (c *Client) GetSystemTelemetry(ctx context.Context) (*SystemInfo, error) { } telemetry, err = rpc.readSystemTelemetryEvent(ctx, defaultRealtimeIntervalSeconds) if err != nil { - return discardRPCSessionForStreamError(err) + return discardRPCSessionForStreamError("reporting.realtime", err) } return rpc.unsubscribe(ctx, subscriptionID) }) @@ -1743,12 +1743,18 @@ func (c *Client) getAppsREST(ctx context.Context) ([]App, error) { } func (c *Client) parseAppsWithStats(ctx context.Context, response []map[string]any) []App { + apps := make([]App, 0, len(response)) + // There is nothing to enrich when app.query returns an empty inventory. + // Subscribing anyway can wait on a stopped Apps service and needlessly + // discard an otherwise healthy, persistent monitoring session. + if len(response) == 0 { + return apps + } statsByApp, err := c.GetAppStats(ctx) if err != nil { statsByApp = nil } - apps := make([]App, 0, len(response)) for _, item := range response { activeWorkloads := readMapAny(item, "active_workloads", "activeWorkloads") @@ -1813,7 +1819,7 @@ func (c *Client) GetAppStats(ctx context.Context) (map[string]AppStats, error) { } stats, err = rpc.readAppStatsEvent(ctx, defaultAppStatsIntervalSeconds) if err != nil { - return discardRPCSessionForStreamError(err) + return discardRPCSessionForStreamError("app.stats", err) } return rpc.unsubscribe(ctx, subscriptionID) }) @@ -1869,7 +1875,7 @@ func (c *Client) GetAppLogs(ctx context.Context, appName, containerID string, ta var reusable bool lines, reusable, err = rpc.readAppLogEvents(ctx, tailLines) if err != nil { - return discardRPCSessionForStreamError(err) + return discardRPCSessionForStreamError("app.container_log_follow", err) } if !reusable { return errRPCStreamSessionConsumed @@ -2335,6 +2341,8 @@ func appendDiskTemperature(out map[string]int, diskName string, value any) { type trueNASRPCClient struct { conn *websocket.Conn nextID int64 + openedAt time.Time + closeOnce sync.Once keepaliveStop chan struct{} keepaliveDone chan struct{} } @@ -2346,7 +2354,7 @@ func (c *trueNASRPCClient) subscribe(ctx context.Context, event string) (string, } subscriptionID = strings.TrimSpace(subscriptionID) if subscriptionID == "" { - return "", &discardRPCSessionError{err: fmt.Errorf("truenas rpc core.subscribe returned an empty subscription id")} + return "", &discardRPCSessionError{method: "core.subscribe", err: fmt.Errorf("truenas rpc core.subscribe returned an empty subscription id")} } return subscriptionID, nil } @@ -2357,7 +2365,7 @@ func (c *trueNASRPCClient) unsubscribe(ctx context.Context, subscriptionID strin return fmt.Errorf("truenas rpc subscription id is required") } if err := c.call(ctx, "core.unsubscribe", []any{subscriptionID}, nil); err != nil { - return &discardRPCSessionError{err: fmt.Errorf("unsubscribe %q: %w", subscriptionID, err)} + return &discardRPCSessionError{method: "core.unsubscribe", err: fmt.Errorf("unsubscribe %q: %w", subscriptionID, err)} } return nil } diff --git a/internal/truenas/transport.go b/internal/truenas/transport.go index 16f1093e0f..bee97cd715 100644 --- a/internal/truenas/transport.go +++ b/internal/truenas/transport.go @@ -12,6 +12,7 @@ import ( "time" "github.com/gorilla/websocket" + "github.com/rs/zerolog/log" ) var errRPCStreamSessionConsumed = errors.New("truenas rpc stream session cannot be reused") @@ -60,7 +61,8 @@ func (c *Client) rpcOperationContext(ctx context.Context) (context.Context, cont } type discardRPCSessionError struct { - err error + method string + err error } func (e *discardRPCSessionError) Error() string { @@ -69,7 +71,7 @@ func (e *discardRPCSessionError) Error() string { func (e *discardRPCSessionError) Unwrap() error { return e.err } -func discardRPCSessionForStreamError(err error) error { +func discardRPCSessionForStreamError(method string, err error) error { if err == nil { return nil } @@ -77,7 +79,7 @@ func discardRPCSessionForStreamError(err error) error { if errors.As(err, &transportErr) { return err } - return &discardRPCSessionError{err: err} + return &discardRPCSessionError{method: method, err: err} } // TransportMode is the connection-local API transport selected for an @@ -326,6 +328,7 @@ func (c *Client) openAuthenticatedRPC(ctx context.Context) (*trueNASRPCClient, s _ = conn.Close() return nil, "", err } + rpc.openedAt = time.Now() rpc.startKeepalive(25 * time.Second) return rpc, authMechanism, nil } @@ -354,13 +357,13 @@ func (c *Client) withRPC(ctx context.Context, operation func(context.Context, *t } if err := operation(ctx, c.rpc); err != nil { if errors.Is(err, errRPCStreamSessionConsumed) { - c.closeRPCLocked() + c.discardRPCLocked("stream_complete", err) c.reconnect = 0 return nil } var discardErr *discardRPCSessionError if errors.As(err, &discardErr) { - c.closeRPCLocked() + c.discardRPCLocked("stream_error", err) c.recordTransportError(err) return err } @@ -369,7 +372,7 @@ func (c *Client) withRPC(ctx context.Context, operation func(context.Context, *t c.recordRPCOperationError(err) return err } - c.closeRPCLocked() + c.discardRPCLocked("transport_error", err) c.recordTransportError(err) if err := c.waitReconnectBackoff(ctx); err != nil { return err @@ -390,9 +393,18 @@ func (c *Client) withRPC(ctx context.Context, operation func(context.Context, *t status.LastError = "" }) if err := operation(ctx, c.rpc); err != nil { + if errors.Is(err, errRPCStreamSessionConsumed) { + c.discardRPCLocked("stream_complete", err) + c.reconnect = 0 + return nil + } + var secondDiscardErr *discardRPCSessionError var secondTransportErr *RPCTransportError - if errors.As(err, &secondTransportErr) { - c.closeRPCLocked() + if errors.As(err, &secondDiscardErr) { + c.discardRPCLocked("stream_error", err) + c.recordTransportError(err) + } else if errors.As(err, &secondTransportErr) { + c.discardRPCLocked("transport_error", err) c.recordTransportError(err) } else { c.recordRPCOperationError(err) @@ -442,7 +454,7 @@ func (c *Client) callRPCWithRetry(ctx context.Context, method string, params any return err } - c.closeRPCLocked() + c.discardRPCLocked("transport_error", err) c.recordTransportError(err) if !retryRead { return fmt.Errorf("truenas action %s transport failed after dispatch; outcome is unknown and Pulse will not replay it: %w", method, err) @@ -470,7 +482,7 @@ func (c *Client) callRPCWithRetry(ctx context.Context, method string, params any if err := c.rpc.call(ctx, method, params, result); err != nil { var secondTransportErr *RPCTransportError if errors.As(err, &secondTransportErr) { - c.closeRPCLocked() + c.discardRPCLocked("transport_error", err) c.recordTransportError(err) } else { c.recordRPCOperationError(err) @@ -502,7 +514,13 @@ func (c *Client) waitReconnectBackoff(ctx context.Context) error { } func (c *Client) closeRPCLocked() { + c.discardRPCLocked("client_closed", nil) +} + +func (c *Client) discardRPCLocked(reason string, err error) { if c.rpc != nil && c.rpc.conn != nil { + method, category := rpcSessionCloseDetails(err) + c.rpc.closeSocket(reason, method, category) c.rpc.close() } c.rpc = nil @@ -531,7 +549,7 @@ func (c *trueNASRPCClient) startKeepalive(interval time.Duration) { if err := c.conn.WriteControl(websocket.PingMessage, nil, time.Now().Add(5*time.Second)); err != nil { // Unblock any in-flight reader; ordinary transport handling owns retry // and status, including the no-replay rule for actions. - _ = c.conn.Close() + c.closeSocket("keepalive_write_error", "", "transport_error") return } } @@ -544,12 +562,68 @@ func (c *trueNASRPCClient) close() { if c.keepaliveStop != nil { close(c.keepaliveStop) } - _ = c.conn.Close() + c.closeSocket("client_closed", "", "") if c.keepaliveDone != nil { <-c.keepaliveDone } } +// Record the first local disposal cause, including an idle keepalive failure +// which may precede the next poll. Never log wire errors, params, subscription +// IDs, endpoints or credentials. These fixed classifications explain a close; +// they do not diagnose the appliance or proxy that caused an exchange to fail. +func (c *trueNASRPCClient) closeSocket(reason, method, category string) { + c.closeOnce.Do(func() { + event := log.Info(). + Str("component", "truenas_rpc"). + Str("action", "close_session"). + Str("reason", reason) + if !c.openedAt.IsZero() { + event.Dur("session_age", time.Since(c.openedAt)) + } + if method != "" { + event.Str("method", method) + } + if category != "" { + event.Str("category", category) + } + event.Msg("Closing TrueNAS RPC session") + _ = c.conn.Close() + }) +} + +func rpcSessionCloseDetails(err error) (method, category string) { + if err == nil { + return "", "" + } + if errors.Is(err, errRPCStreamSessionConsumed) { + return "app.container_log_follow", "idle_timeout" + } + var discardErr *discardRPCSessionError + var transportErr *RPCTransportError + var rpcErr *RPCError + if errors.As(err, &discardErr) { + method = discardErr.method + } else if errors.As(err, &transportErr) { + method = transportErr.Method + } else if errors.As(err, &rpcErr) { + method = rpcErr.Method + } + switch { + case errors.Is(err, context.Canceled): + category = "cancelled" + case errors.Is(err, context.DeadlineExceeded) || isTimeoutError(err): + category = "timeout" + case errors.As(err, &rpcErr): + category = "method_error" + case errors.As(err, &transportErr): + category = "transport_error" + default: + category = "protocol_error" + } + return method, category +} + func (c *Client) recordTransportError(err error) { message := c.sanitizeTransportError(err) c.updateTransportStatus(func(status *TransportStatus) { diff --git a/internal/truenas/transport_test.go b/internal/truenas/transport_test.go index 00d284e872..cdbf583517 100644 --- a/internal/truenas/transport_test.go +++ b/internal/truenas/transport_test.go @@ -1,6 +1,7 @@ package truenas import ( + "bytes" "context" "encoding/json" "errors" @@ -16,6 +17,8 @@ import ( "time" "github.com/gorilla/websocket" + "github.com/rs/zerolog" + "github.com/rs/zerolog/log" ) type protocolFixtureReply struct { @@ -786,8 +789,8 @@ func TestJSONRPCFetchSnapshotUsesPublishedTrueNAS26MethodsWithoutREST(t *testing subscribeCalls := methodCalls["core.subscribe"] unsubscribeCalls := methodCalls["core.unsubscribe"] methodCallsMu.Unlock() - if subscribeCalls != 2 || unsubscribeCalls != 2 { - t.Fatalf("subscription lifecycle calls: subscribe=%d unsubscribe=%d, want 2/2", subscribeCalls, unsubscribeCalls) + if subscribeCalls != 1 || unsubscribeCalls != 1 { + t.Fatalf("subscription lifecycle calls: subscribe=%d unsubscribe=%d, want 1/1 (realtime only for empty apps)", subscribeCalls, unsubscribeCalls) } if fixture.sessions.Load() != 1 || fixture.restRequests.Load() != 0 { t.Fatalf("snapshot sessions=%d REST=%d, want one modern session and no REST", @@ -1419,3 +1422,272 @@ func TestJSONRPCSnapshotContinuesAfterTelemetryTimeout(t *testing.T) { t.Fatalf("inventory did not recover on RPC after discarding the stream: sessions=%d rest=%d", fixture.sessions.Load(), fixture.restRequests.Load()) } } + +func TestJSONRPCEmptyAppsDoNotSubscribeToStats(t *testing.T) { + var appQueries, statsSubscriptions atomic.Int32 + fixture := newProtocolFixture(t, func(_ int, request trueNASRPCRequest) protocolFixtureReply { + switch request.Method { + case "auth.login_ex": + return protocolFixtureReply{result: map[string]any{"response_type": "SUCCESS"}} + case "app.query": + if appQueries.Add(1) == 1 { + return protocolFixtureReply{result: nil} + } + return protocolFixtureReply{result: []any{}} + case "core.subscribe": + statsSubscriptions.Add(1) + // Deliberately silent stats on an appliance with no apps. The final + // client must never issue this request, even with a cancellation-only + // caller context like the production poller. + return protocolFixtureReply{result: "unavailable-apps"} + case "system.info": + return protocolFixtureReply{result: map[string]any{"hostname": "empty-app-nas", "version": "TrueNAS-SCALE-25.04.2.6"}} + default: + t.Errorf("unexpected method %s", request.Method) + return protocolFixtureReply{close: true} + } + }, nil) + client := protocolFixtureClient(t, fixture.server.URL, ClientConfig{APIKey: "fixture-key", Username: "fixture-user", Timeout: 100 * time.Millisecond}) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + for i := 0; i < 2; i++ { + apps, err := client.GetApps(ctx) + if err != nil || apps == nil || len(apps) != 0 { + t.Fatalf("empty app query %d did not preserve an empty successful inventory", i) + } + } + if statsSubscriptions.Load() != 0 { + t.Fatal("empty inventory still issued a stats subscription") + } + if _, err := client.GetSystemInfo(ctx); err != nil { + t.Fatal(err) + } + if fixture.sessions.Load() != 1 || fixture.restRequests.Load() != 0 || !client.TransportStatus().Connected { + t.Fatal("empty apps discarded the healthy authenticated session or changed transport") + } +} + +func TestLegacyRESTEmptyAppsDoNotNegotiateStats(t *testing.T) { + var appQueries atomic.Int32 + fixture := newProtocolFixture(t, func(_ int, request trueNASRPCRequest) protocolFixtureReply { + t.Errorf("empty legacy app inventory issued RPC method %s", request.Method) + return protocolFixtureReply{close: true} + }, func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/v2.0/app" { + t.Errorf("unexpected legacy path %s", r.URL.Path) + return + } + appQueries.Add(1) + _ = json.NewEncoder(w).Encode([]any{}) + }) + client := protocolFixtureClient(t, fixture.server.URL, ClientConfig{APIKey: "fixture-key"}) + // Negotiation is covered separately; isolate the already-selected legacy + // inventory path here, so no attempted JSON-RPC stats lookup is hidden. + client.mode = TransportLegacyREST + apps, err := client.GetApps(context.Background()) + if err != nil || apps == nil || len(apps) != 0 || appQueries.Load() != 1 || fixture.sessions.Load() != 0 { + t.Fatal("empty legacy app inventory did not stay a single REST read") + } +} + +func TestJSONRPCStreamRetryDiscardsUnusableSecondSession(t *testing.T) { + for _, failure := range []string{"malformed_event", "empty_subscription", "unsubscribe_error"} { + t.Run(failure, func(t *testing.T) { + var subscribes atomic.Int32 + fixture := newProtocolFixture(t, func(session int, request trueNASRPCRequest) protocolFixtureReply { + switch request.Method { + case "auth.login_ex": + return protocolFixtureReply{result: map[string]any{"response_type": "SUCCESS"}} + case "core.subscribe": + subscribes.Add(1) + if session == 1 { + return protocolFixtureReply{close: true} + } + if failure == "empty_subscription" { + return protocolFixtureReply{result: ""} + } + var fields any = []any{map[string]any{"app_name": "fixture-app", "cpu_usage": 3}} + if failure == "malformed_event" { + fields = "invalid-fields" + } + return protocolFixtureReply{result: "private-subscription-id", notifications: []protocolFixtureNotification{{method: "collection_update", params: map[string]any{"collection": "app.stats:{\"interval\":2}", "fields": fields}}}} + case "core.unsubscribe": + return protocolFixtureReply{err: &trueNASRPCError{Code: -32001, Message: "fixture-method-error"}} + case "pool.query": + if session != 3 { + t.Error("next read reused an unusable second stream session") + } + return protocolFixtureReply{result: []any{}} + default: + t.Errorf("unexpected method %s", request.Method) + return protocolFixtureReply{close: true} + } + }, nil) + client := protocolFixtureClient(t, fixture.server.URL, ClientConfig{APIKey: "fixture-key", Username: "fixture-user"}) + if _, err := client.GetAppStats(context.Background()); err == nil { + t.Fatal("bad second stream session appeared successful") + } + if client.TransportStatus().Connected { + t.Fatal("unusable second stream session remained connected") + } + if client.rpc != nil { + t.Fatal("unusable second stream session was retained") + } + if _, err := client.GetPools(context.Background()); err != nil { + t.Fatal(err) + } + if subscribes.Load() != 2 || fixture.sessions.Load() != 3 || fixture.restRequests.Load() != 0 { + t.Fatal("stream retry/disposal changed its retry limit or downgraded transport") + } + }) + } +} + +func TestJSONRPCStreamRetryReturnsCompletedLogTail(t *testing.T) { + fixture := newProtocolFixture(t, func(session int, request trueNASRPCRequest) protocolFixtureReply { + switch request.Method { + case "auth.login_ex": + return protocolFixtureReply{result: map[string]any{"response_type": "SUCCESS"}} + case "core.subscribe": + if session == 1 { + return protocolFixtureReply{close: true} + } + return protocolFixtureReply{result: "fixture-log-sub", notifications: []protocolFixtureNotification{{method: "collection_update", params: map[string]any{"collection": "app.container_log_follow:{}", "fields": []any{map[string]any{"data": "fixture-line"}}}}}} + case "pool.query": + if session != 3 { + t.Error("timed-out log socket was reused") + } + return protocolFixtureReply{result: []any{}} + default: + t.Errorf("unexpected method %s", request.Method) + return protocolFixtureReply{close: true} + } + }, nil) + client := protocolFixtureClient(t, fixture.server.URL, ClientConfig{APIKey: "fixture-key", Username: "fixture-user"}) + lines, err := client.GetAppLogs(context.Background(), "fixture-app", "fixture-container", 100) + if err != nil || len(lines) != 1 || lines[0].Data != "fixture-line" { + t.Fatal("completed retry lost its log tail or returned a consumed-session sentinel") + } + if client.TransportStatus().Connected || client.rpc != nil { + t.Fatal("completed retry kept a timed-out log socket") + } + if _, err := client.GetPools(context.Background()); err != nil { + t.Fatal(err) + } + if fixture.sessions.Load() != 3 || fixture.restRequests.Load() != 0 { + t.Fatal("next read did not open exactly one fresh modern session") + } +} + +func TestRPCSessionCloseDetails(t *testing.T) { + for _, tc := range []struct { + name string + err error + method, category string + }{ + {"explicit", nil, "", ""}, + {"idle tail", errRPCStreamSessionConsumed, "app.container_log_follow", "idle_timeout"}, + {"timeout", &RPCTransportError{Method: "pool.query", Phase: "read", Err: context.DeadlineExceeded}, "pool.query", "timeout"}, + {"cancel", &RPCTransportError{Method: "app.start", Phase: "read", Err: context.Canceled}, "app.start", "cancelled"}, + {"transport", &RPCTransportError{Method: "system.info", Phase: "read", Err: errors.New("private-error-text")}, "system.info", "transport_error"}, + {"stream decode", &discardRPCSessionError{method: "app.stats", err: errors.New("private-event-text")}, "app.stats", "protocol_error"}, + {"unsubscribe refusal", &discardRPCSessionError{method: "core.unsubscribe", err: &RPCError{Method: "core.unsubscribe", Message: "private-wire-message"}}, "core.unsubscribe", "method_error"}, + } { + t.Run(tc.name, func(t *testing.T) { + method, category := rpcSessionCloseDetails(tc.err) + if method != tc.method || category != tc.category { + t.Fatalf("close classification = %s/%s, want %s/%s", method, category, tc.method, tc.category) + } + }) + } +} + +func captureRPCCloseLogs(t *testing.T) *bytes.Buffer { + t.Helper() + var output bytes.Buffer + previous := log.Logger + log.Logger = zerolog.New(&output) + t.Cleanup(func() { log.Logger = previous }) + return &output +} + +func rpcCloseLogRecords(t *testing.T, output *bytes.Buffer) []map[string]any { + t.Helper() + var records []map[string]any + decoder := json.NewDecoder(bytes.NewReader(output.Bytes())) + for decoder.More() { + var record map[string]any + if err := decoder.Decode(&record); err != nil { + t.Fatal("close log is not structured JSON") + } + records = append(records, record) + } + return records +} + +func TestRPCSessionCloseLogIsBoundedAndSecretFree(t *testing.T) { + output := captureRPCCloseLogs(t) + fixture := newProtocolFixture(t, func(_ int, request trueNASRPCRequest) protocolFixtureReply { + switch request.Method { + case "auth.login_ex": + return protocolFixtureReply{result: map[string]any{"response_type": "SUCCESS"}} + case "core.subscribe": + return protocolFixtureReply{result: "private-subscription-id", notifications: []protocolFixtureNotification{{method: "collection_update", params: map[string]any{"collection": "app.stats:{\"interval\":2}", "fields": []any{}}}}} + case "core.unsubscribe": + return protocolFixtureReply{err: &trueNASRPCError{Code: -32001, Message: "private-wire-message private-api-key private-password private-subscription-id"}} + default: + return protocolFixtureReply{close: true} + } + }, nil) + client := protocolFixtureClient(t, fixture.server.URL, ClientConfig{APIKey: "private-api-key", Password: "private-password", Username: "private-owner"}) + if _, err := client.GetAppStats(context.Background()); err == nil { + t.Fatal("fixture did not fail its unsubscribe") + } + client.Close() + records := rpcCloseLogRecords(t, output) + if len(records) != 1 { + t.Fatalf("close records = %d, want exactly one for disposal and subsequent Close", len(records)) + } + record := records[0] + if record["reason"] != "stream_error" || record["method"] != "core.unsubscribe" || record["category"] != "method_error" || record["component"] != "truenas_rpc" || record["action"] != "close_session" { + t.Fatal("close log lost its fixed local cause") + } + if _, ok := record["session_age"].(float64); !ok { + t.Fatal("close log lost the authenticated session age") + } + for _, private := range []string{"private-api-key", "private-password", "private-owner", "private-wire-message", "private-subscription-id", fixture.server.URL} { + if bytes.Contains(output.Bytes(), []byte(private)) { + t.Fatal("close log contains private wire/configuration data") + } + } + for key := range record { + switch key { + case "component", "action", "reason", "method", "category", "session_age", "level", "message": + default: + t.Fatalf("unexpected close log field %s", key) + } + } +} + +func TestRPCSessionCloseLogKeepsFirstKeepaliveCause(t *testing.T) { + output := captureRPCCloseLogs(t) + fixture := newProtocolFixture(t, func(_ int, _ trueNASRPCRequest) protocolFixtureReply { return protocolFixtureReply{result: true} }, nil) + client := protocolFixtureClient(t, fixture.server.URL, ClientConfig{}) + conn, err := client.dialRPC(context.Background()) + if err != nil { + t.Fatal(err) + } + rpc := &trueNASRPCClient{conn: conn, nextID: 1, openedAt: time.Now()} + rpc.startKeepalive(time.Millisecond) + _ = conn.Close() // Inject a control-write failure, without an RPC payload. + select { + case <-rpc.keepaliveDone: + case <-time.After(time.Second): + t.Fatal("failed keepalive did not stop") + } + rpc.close() + records := rpcCloseLogRecords(t, output) + if len(records) != 1 || records[0]["reason"] != "keepalive_write_error" || records[0]["category"] != "transport_error" { + t.Fatal("later client disposal overwrote or duplicated the first keepalive close cause") + } +} From 34f73590bbb72c01a3e16e6085227849556a2b1d Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Fri, 2 Oct 2026 22:34:17 +0100 Subject: [PATCH 3/5] Keep pool disk facts on the owning storage host Repeated device paths and UnRAID group names must not attach foreign SMART, temperatures or topology counts to a pool. Preserve canonical children, require host or scoped native ownership for inferred membership, and compare exact device identifiers. Cover live target changes and real desktop/phone presentations. Change-source: pulse-maintainer --- .../subsystems/frontend-primitives.md | 17 ++ .../internal/subsystems/storage-recovery.md | 23 +++ .../browser-tests/pool-ownership.cjs | 177 ++++++++++++++++++ .../browser-tests/pool-ownership.html | 12 ++ .../browser-tests/pool-ownership.tsx | 123 ++++++++++++ frontend-modern/browser-verification.json | 52 ++--- .../StoragePoolDetail.ownership.test.tsx | 87 +++++++++ .../__tests__/StoragePoolDetail.test.tsx | 1 + .../useStoragePoolDetailModel.test.ts | 2 + ...gePoolDetailPresentation.branchcov.test.ts | 8 +- .../storagePoolDetailPresentation.test.ts | 5 + .../storagePoolDiskOwnership.test.ts | 170 +++++++++++++++++ .../storageBackups/storageAdapters.ts | 4 + .../storagePoolDetailPresentation.ts | 31 ++- 14 files changed, 674 insertions(+), 38 deletions(-) create mode 100644 frontend-modern/browser-tests/pool-ownership.cjs create mode 100644 frontend-modern/browser-tests/pool-ownership.html create mode 100644 frontend-modern/browser-tests/pool-ownership.tsx create mode 100644 frontend-modern/src/components/Storage/__tests__/StoragePoolDetail.ownership.test.tsx create mode 100644 frontend-modern/src/features/storageBackups/__tests__/storagePoolDiskOwnership.test.ts diff --git a/docs/release-control/v6/internal/subsystems/frontend-primitives.md b/docs/release-control/v6/internal/subsystems/frontend-primitives.md index e0d9578876..6a0e57516a 100644 --- a/docs/release-control/v6/internal/subsystems/frontend-primitives.md +++ b/docs/release-control/v6/internal/subsystems/frontend-primitives.md @@ -7752,3 +7752,20 @@ production disk/pool details and API transport in phone Chromium/WebKit, native vertical panning in Chromium, and desktop mouse/keyboard controls. Synthetic sample presentation is not native collector, physical device, assistive-device or installed whole-report acceptance. + +### Shared Storage presenters preserve disk ownership + +Pool-detail presenters under `features/storageBackups/` must not use repeated +device paths, UnRAID group labels or host display names as fleet-wide identity. +Direct pool children are retained; inferred membership requires the same +canonical host parent, or a complete native Proxmox instance/node pair when +parents are missing. Known parent conflicts win over native names. Full ZFS +device identifiers may normalize an optional `/dev/` prefix but must not match +arbitrary suffixes. Missing ownership produces no invented linked-disk facts. +The storage adapter preserves the instance needed for that presentation scope; +this does not rewrite canonical resources or History targets. Existing keyed +pool details must update linked disks and local health through target/snapshot +replacement without retaining peer-host rows. The mounted +`StoragePoolDetail.ownership.test.tsx` and production-component browser fixture +`browser-tests/pool-ownership.cjs` verify desktop/phone presentation and History +coexistence; they are not field collection or native recovery evidence. diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index 20667521cd..97ffabafe7 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -6311,3 +6311,26 @@ units, collection, retention, access or recovery boundary. The production disk/pool fixture `frontend-modern/browser-tests/history-touch.cjs` and mounted `HistoryChart.test.tsx` regressions verify presentation/input behaviour only, not live appliance collection or recovery success. + +### Pool-to-physical-disk ownership in Storage details + +`storagePoolDetailPresentation.ts` must scope inferred ZFS device and UnRAID +array/cache-group membership to the owning host. An explicit disk parent equal +to the canonical pool ID or its retained resource reference remains a direct +association. Otherwise, matching canonical host parents establishes scope; +conflicting parents cannot be overridden by names, paths or group labels. +When canonical parents are unavailable, only a complete matching native +Proxmox instance/node pair establishes scope. `storageAdapters.ts` retains that +instance alongside the native node. Display names and unscoped hostnames alone +are insufficient. Unknown ownership must not present another host's SMART, +temperature, counters or topology counts as this pool's evidence. + +ZFS device matching compares full identifiers with only the optional `/dev/` +prefix normalized, not arbitrary suffixes or invented partition associations. +This is read-only presentation: no collector, canonical server identity, +metrics target, health threshold, persistence or recovery authority changes. +`storagePoolDiskOwnership.test.ts`, the mounted +`StoragePoolDetail.ownership.test.tsx` and `browser-tests/pool-ownership.cjs` +cover repeated paths/groups, direct children, scoped legacy snapshots, missing +ownership and live target replacement. Synthetic presentation proof does not +establish native appliance, installation or recovery acceptance. diff --git a/frontend-modern/browser-tests/pool-ownership.cjs b/frontend-modern/browser-tests/pool-ownership.cjs new file mode 100644 index 0000000000..7e9a879912 --- /dev/null +++ b/frontend-modern/browser-tests/pool-ownership.cjs @@ -0,0 +1,177 @@ +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const { chromium, webkit } = require('playwright'); +const parent = false; +(async () => { + const root = '/workspace/frontend-modern'; + process.chdir(root); + const artifacts = path.join(root, 'node_modules/pool-ownership-proof'); + fs.mkdirSync(artifacts, { recursive: true }); + const { createServer } = await import(path.join(root, 'node_modules/vite/dist/node/index.js')); + const server = await createServer({ + root, + configFile: path.join(root, 'vite.config.ts'), + cacheDir: fs.mkdtempSync(path.join(artifacts, 'vite-')), + server: { host: '127.0.0.1', port: 5234, strictPort: true }, + }); + const playwright = require('playwright/package.json').version; + const expected = JSON.parse( + fs.readFileSync('/workspace/tests/integration/package-lock.json', 'utf8'), + ).packages['node_modules/@playwright/test'].version; + assert.equal(playwright, expected); + const observations = []; + let browser; + try { + await server.listen(); + for (const [name, engine, phone] of [ + ['chromium-desktop', chromium, false], + ['webkit-phone', webkit, true], + ]) { + browser = await engine.launch( + engine === chromium + ? { headless: true, channel: 'chromium', args: ['--no-sandbox'] } + : { headless: true }, + ); + const page = await browser.newPage({ + viewport: phone ? { width: 390, height: 844 } : { width: 1365, height: 900 }, + hasTouch: phone, + isMobile: phone, + }); + const requests = [], + errors = []; + page.on('pageerror', (error) => errors.push(error.message)); + await page.route('**/*', (route) => { + const url = new URL(route.request().url()); + if (url.origin !== 'http://127.0.0.1:5234') return route.abort(); + if (!url.pathname.startsWith('/api/')) return route.continue(); + if (url.pathname === '/api/metrics-store/history') { + requests.push(Object.fromEntries(url.searchParams)); + return route.fulfill({ + json: { + points: [{ timestamp: 1790935200000, value: 25, min: 25, max: 25 }], + source: 'store', + }, + }); + } + return route.fulfill({ + json: + url.pathname === '/api/license/runtime-capabilities' + ? { + capabilities: [], + limits: [], + max_history_days: 7, + hosted_mode: false, + runtime: { build: 'community' }, + blocked_capabilities: [], + } + : { data: [], enabled: false }, + }); + }); + await page.goto('http://127.0.0.1:5234/browser-tests/pool-ownership.html'); + if (phone) await page.evaluate(() => document.documentElement.classList.add('dark')); + const detail = page.getByRole('region', { name: 'Pool detail' }); + const phases = parent + ? [ + ['ZFS host A', 4, null], + ['UnRAID host A', 3, null], + ] + : [ + ['ZFS host A', 1, 'Host A disk'], + ['ZFS host B', 1, 'Host B disk'], + ['Missing ownership', 0, null], + ['Direct child', 1, 'Host A disk'], + ['UnRAID host A', 1, 'Host A disk'], + ['UnRAID host B', 1, 'Host B disk'], + ]; + for (const [phase, count, model] of phases) { + await page.getByRole('button', { name: phase, exact: true }).click(); + const text = await detail.textContent(); + if (count) { + await detail.getByText(`Physical Disks (${count})`, { exact: true }).waitFor(); + if (model) { + assert.ok(text.includes(model)); + assert.ok(!text.includes(model === 'Host A disk' ? 'Host B disk' : 'Host A disk')); + assert.ok(!text.includes('Unowned disk') && !text.includes('Suffix collision disk')); + assert.equal(text.includes('99 errors'), model === 'Host B disk'); + assert.ok(text.includes(model === 'Host B disk' ? '82°C' : '34°C')); + } else + assert.ok( + text.includes('Host B disk') && text.includes('99 errors'), + 'parent must expose foreign health', + ); + } else assert.equal(await detail.getByText(/Physical Disks/).count(), 0); + const dimensions = await page.evaluate(() => ({ + scroll: document.documentElement.scrollWidth, + width: innerWidth, + })); + assert.ok(dimensions.scroll <= dimensions.width + 1, JSON.stringify(dimensions)); + const screenshot = `${parent ? 'parent-' : ''}${name}-${phase.toLowerCase().replaceAll(' ', '-')}.png`; + await page.screenshot({ path: path.join(artifacts, screenshot), fullPage: true }); + observations.push({ + name, + phase, + browser: browser.version(), + text, + dimensions, + screenshot, + }); + } + if (!parent) { + await page.getByRole('button', { name: 'ZFS host A', exact: true }).click(); + await detail.getByRole('tab', { name: 'History', exact: true }).click(); + await detail.getByRole('img', { name: 'Usage chart', exact: true }).waitFor(); + await page.waitForFunction(() => + [...document.querySelectorAll('canvas')].some((el) => + document + .getElementById(el.getAttribute('aria-describedby')) + ?.textContent.includes('1 data point'), + ), + ); + assert.ok( + requests.some( + (request) => + request.resourceId === 'history-a' && + request.metric === 'usage' && + request.resourceType === 'storage', + ), + ); + await page.getByRole('button', { name: 'ZFS host B', exact: true }).click(); + await page.waitForFunction(() => + [...document.querySelectorAll('canvas')].some((el) => + document + .getElementById(el.getAttribute('aria-describedby')) + ?.textContent.includes('1 data point'), + ), + ); + assert.ok( + requests.some( + (request) => + request.resourceId === 'history-b' && + request.metric === 'usage' && + request.resourceType === 'storage', + ), + ); + await detail.getByRole('tab', { name: 'Overview', exact: true }).click(); + await detail.getByText('Physical Disks (1)', { exact: true }).waitFor(); + assert.equal(await detail.getByText('Host A disk', { exact: true }).count(), 0); + assert.equal(await detail.getByText('Host B disk', { exact: true }).count(), 1); + } + assert.deepEqual(errors, []); + observations.push({ name, phase: 'history-requests', requests, errors }); + await browser.close(); + browser = null; + } + fs.writeFileSync( + path.join(artifacts, parent ? 'parent.json' : 'result.json'), + JSON.stringify({ playwright, expected, parent, observations }, null, 2), + ); + console.log(JSON.stringify({ result: 'passed', parent, observations: observations.length })); + } finally { + if (browser) await browser.close(); + await server.close(); + } +})().catch((error) => { + console.error(error); + process.exitCode = 1; +}); diff --git a/frontend-modern/browser-tests/pool-ownership.html b/frontend-modern/browser-tests/pool-ownership.html new file mode 100644 index 0000000000..a215a7dc44 --- /dev/null +++ b/frontend-modern/browser-tests/pool-ownership.html @@ -0,0 +1,12 @@ + + + + + + Pool ownership verification + + +

+ + + diff --git a/frontend-modern/browser-tests/pool-ownership.tsx b/frontend-modern/browser-tests/pool-ownership.tsx new file mode 100644 index 0000000000..9f26743e96 --- /dev/null +++ b/frontend-modern/browser-tests/pool-ownership.tsx @@ -0,0 +1,123 @@ +import { createMemo, createSignal, For } from 'solid-js'; +import { render } from 'solid-js/web'; +import { StoragePoolDetail } from '../src/components/Storage/StoragePoolDetail'; +import { buildStorageRecords } from '../src/features/storageBackups/storageAdapters'; +import type { State } from '../src/types/api'; +import type { Resource } from '../src/types/resource'; +import '../src/index.css'; + +const modes = [ + 'ZFS host A', + 'ZFS host B', + 'UnRAID host A', + 'UnRAID host B', + 'Missing ownership', + 'Direct child', +]; +const [mode, setMode] = createSignal(modes[0]); +const resource = createMemo(() => { + const unraid = mode().startsWith('UnRAID'); + const b = mode().endsWith('B'); + const unknown = mode() === 'Missing ownership'; + return { + id: b ? 'pool-b' : 'pool-a', + type: 'storage', + name: unraid ? 'Array' : 'tank', + displayName: 'tank', + platformType: unraid ? 'unraid' : 'truenas', + platformId: 'fixture', + sourceType: 'api', + parentId: unknown ? undefined : b ? 'host-b' : 'host-a', + parentName: 'Storage host', + status: 'online', + lastSeen: Date.now(), + metricsTarget: { resourceType: 'storage', resourceId: b ? 'history-b' : 'history-a' }, + disk: { current: 25, total: 1024, used: 256, free: 768 }, + storage: unraid + ? { type: 'unraid-array', platform: 'unraid' } + : { + type: 'zfs-pool', + isZfs: true, + zfsPool: { + name: 'tank', + state: 'ONLINE', + readErrors: 0, + writeErrors: 0, + checksumErrors: 0, + devices: [{ name: 'sda', type: 'disk', state: 'ONLINE' }], + }, + }, + } as Resource; +}); +const record = createMemo( + () => buildStorageRecords({ state: {} as State, resources: [resource()] })[0], +); +const disks = createMemo(() => + [ + ['a', 'host-a', 'Host A disk', '/dev/sda', 34, 0], + ['b', 'host-b', 'Host B disk', '/dev/sda', 82, 99], + ['unknown', undefined, 'Unowned disk', '/dev/sda', 70, 0], + ['suffix', 'host-a', 'Suffix collision disk', '/dev/not-sda', 66, 0], + ].map( + ([id, owner, model, devPath, temperature, errorCount]) => + ({ + id: `disk-${id}`, + type: 'physical_disk', + name: model, + displayName: model, + platformType: 'truenas', + platformId: 'fixture', + sourceType: 'api', + status: 'online', + lastSeen: Date.now(), + parentId: mode() === 'Direct child' && id === 'a' ? 'pool-a' : owner, + parentName: 'Storage host', + physicalDisk: { + devPath, + model, + temperature, + errorCount, + storageGroup: id === 'suffix' ? 'other-group' : 'unraid-array', + storageRole: 'data', + sizeBytes: 1024, + storageState: 'online', + }, + }) as Resource, + ), +); + +render( + () => ( +
+

Pool ownership verification

+

+ Repeated disk names and groups across synthetic hosts, using the production Storage detail. +

+ +
+

{mode()}

+ + + + +
+
+
+ ), + document.getElementById('root')!, +); diff --git a/frontend-modern/browser-verification.json b/frontend-modern/browser-verification.json index 395e434f7a..c4dc983bda 100644 --- a/frontend-modern/browser-verification.json +++ b/frontend-modern/browser-verification.json @@ -1,19 +1,17 @@ { "version": 1, - "base_sha": "e5d0843851a3837a90d71afaced3a009c0606bbf", - "verified_at": "2026-10-02T20:44:41.220410Z", + "base_sha": "689079622dfa8fb472349ad13c36ad2068f0c38e", + "verified_at": "2026-10-02T21:27:29.856838Z", "result": "passed", "changed_paths": [ - "frontend-modern/src/components/shared/HistoryChart.tsx", - "frontend-modern/src/components/shared/useHistoryChartState.ts" + "frontend-modern/src/features/storageBackups/storageAdapters.ts", + "frontend-modern/src/features/storageBackups/storagePoolDetailPresentation.ts" ], "content_sha256": { - "frontend-modern/src/components/shared/HistoryChart.tsx": "74fb7e6b466e1c2b2d2efb13e0a124d6c301896a7c9423432e903908cc9ee25b", - "frontend-modern/src/components/shared/useHistoryChartState.ts": "d6e8317019c8dc616bd9a4d3c6c71cb5329297ca0341e6ba28a6cc4c84db36b0" + "frontend-modern/src/features/storageBackups/storageAdapters.ts": "7933ab99a1cfdf35bcebf0f351ec2b6a8c486b1281a9a3daa36f828fa657c52e", + "frontend-modern/src/features/storageBackups/storagePoolDetailPresentation.ts": "dd97511242f9375a725f69b3f8f40b2aee19998bdcdd2e82ce996d27f9fc519b" }, - "routes": [ - "/browser-tests/history-touch.html" - ], + "routes": ["/browser-tests/pool-ownership.html"], "viewports": [ { "width": 1365, @@ -25,33 +23,19 @@ } ], "states": [ - "Production disk/pool details with stored multi-point, measured-zero singleton and empty history; 19 observed presentations across desktop Chromium and phone Chromium/WebKit", - "Actual touchscreen first/middle/last taps, shared I/O group focus transfer, keyboard navigation/exit and real mouse coexistence; phone WebKit dark-class presentation" + "Production StoragePoolDetail through the real storage adapter; desktop Chromium light and phone WebKit dark; ZFS/UnRAID host A and B, missing ownership and direct children", + "12 recorded presentation states, plus per-engine History target replacement and Overview return" ], "interactions": [ - "Native touchscreen taps select the actual reading at that position instead of the latest sample; touch compatibility focus/mouse movement/leave cannot overwrite it", - "Primary contact cancellation, non-tap gesture rejection and native Chromium vertical pan with room-to-scroll precondition; no prevented touch default, pointer capture or touch-action change", - "Desktop/phone keyboard Home/Right/End/Escape and focus exit, pointer announcements remain quiet, range replacement, zero/empty responses and no horizontal overflow", - "Predecessor native Chromium/WebKit first tap near 10% instead displays 30%; final first/middle/last taps display 10/20/30%. Inspected predecessor and final phone, zero/empty and desktop screenshots" + "Replace the selected host in the mounted detail: a remote 82\u00b0C/99-error disk appears only on its owning host; local temperature and errors remain visible", + "Missing ownership omits inferred Physical Disks without removing ZFS information; direct children remain", + "History range and canonical history-a/history-b requests stay isolated while selected hosts change; no horizontal page overflow" ], - "command": "pulse-worker-browser frontend-modern/browser-tests/history-touch.cjs", - "browser_versions": { + "observations": { "playwright": "1.56.1", - "chromium": "141.0.7390.37", - "webkit": "26.0" - }, - "artifacts": [ - "browser/result.json", - "browser/chromium-phone-first.png", - "browser/webkit-phone-first.png", - "browser/webkit-phone-middle.png", - "browser/webkit-phone-pool-first.png", - "browser/webkit-phone-pool-zero-singleton.png", - "browser/webkit-phone-pool-empty.png", - "browser/chromium-desktop-middle.png", - "browser/parent.json", - "browser/parent-chromium-phone-first.png", - "browser/parent-webkit-phone-first.png" - ], - "limitations": "Synthetic stored APIs, not native PBS/TrueNAS collection, physical phones or assistive-device speech. Native pan is Chromium-only; no physical/WebKit pan, pinch-zoom or whole-app theme-transition claim. Initial fixture and pan-driver failures are retained separately." + "expected_playwright": "1.56.1", + "result": "browser/result.json", + "parent": "browser/parent.json", + "driver": "frontend-modern/browser-tests/pool-ownership.cjs" + } } diff --git a/frontend-modern/src/components/Storage/__tests__/StoragePoolDetail.ownership.test.tsx b/frontend-modern/src/components/Storage/__tests__/StoragePoolDetail.ownership.test.tsx new file mode 100644 index 0000000000..b3a08bed5f --- /dev/null +++ b/frontend-modern/src/components/Storage/__tests__/StoragePoolDetail.ownership.test.tsx @@ -0,0 +1,87 @@ +import { createSignal } from 'solid-js'; +import { fireEvent, render, screen } from '@solidjs/testing-library'; +import { describe, expect, it, vi } from 'vitest'; +import { StoragePoolDetail } from '../StoragePoolDetail'; +import { buildStorageRecords } from '@/features/storageBackups/storageAdapters'; +import type { Resource } from '@/types/resource'; +import type { State } from '@/types/api'; + +vi.mock('@/components/shared/HistoryChart', () => ({ + HistoryChart: () =>
History fixture
, +})); + +const pool = (owner: string | undefined): Resource => + ({ + id: 'pool', + name: 'tank', + type: 'storage', + platformType: 'truenas', + parentId: owner, + parentName: 'Identical label', + storage: { + type: 'zfs-pool', + isZfs: true, + zfsPool: { + name: 'tank', + state: 'ONLINE', + readErrors: 0, + writeErrors: 0, + checksumErrors: 0, + devices: [{ name: 'sda', type: 'disk', state: 'ONLINE' }], + }, + }, + }) as Resource; + +const disk = (id: string, owner: string, model: string, errors = 0): Resource => + ({ + id, + name: model, + type: 'physical_disk', + platformType: 'truenas', + parentId: owner, + parentName: 'Identical label', + physicalDisk: { devPath: '/dev/sda', model, errorCount: errors, temperature: errors ? 82 : 34 }, + }) as Resource; + +describe('Storage pool ownership through live snapshots', () => { + it('updates mounted ownership, local warnings and removal without leaking peer disks', () => { + const [resource, setResource] = createSignal(pool('host-a')); + const [disks, setDisks] = createSignal([ + disk('a', 'host-a', 'Local disk'), + disk('b', 'host-b', 'Peer disk', 99), + ]); + const record = () => buildStorageRecords({ state: {} as State, resources: [resource()] })[0]; + render(() => ( + + + + +
+ )); + + expect(screen.getByText('Physical Disks (1)')).toBeVisible(); + expect(screen.getByText('Local disk')).toBeVisible(); + expect(screen.queryByText('Peer disk')).not.toBeInTheDocument(); + expect(screen.queryByText('99 errors')).not.toBeInTheDocument(); + + setResource(pool('host-b')); + expect(screen.queryByText('Local disk')).not.toBeInTheDocument(); + expect(screen.getByText('Peer disk')).toBeVisible(); + expect(screen.getByText('99 errors')).toBeVisible(); + expect(screen.getByText('82°C')).toBeVisible(); + fireEvent.click(screen.getByRole('tab', { name: 'History' })); + fireEvent.click(screen.getByRole('tab', { name: 'Overview' })); + expect(screen.getByText('Peer disk')).toBeVisible(); + + setResource(pool(undefined)); + expect(screen.queryByText(/Physical Disks/)).not.toBeInTheDocument(); + expect(screen.getByText('ZFS Pool')).toBeVisible(); + setResource(pool('host-a')); + setDisks([disk('a', 'host-a', 'Local disk', 4)]); + expect(screen.getByText('Local disk')).toBeVisible(); + expect(screen.getByText('4 errors')).toBeVisible(); + setDisks([]); + expect(screen.queryByText(/Physical Disks/)).not.toBeInTheDocument(); + expect(screen.queryByText('4 errors')).not.toBeInTheDocument(); + }); +}); diff --git a/frontend-modern/src/components/Storage/__tests__/StoragePoolDetail.test.tsx b/frontend-modern/src/components/Storage/__tests__/StoragePoolDetail.test.tsx index d0f472d70f..4652191e6c 100644 --- a/frontend-modern/src/components/Storage/__tests__/StoragePoolDetail.test.tsx +++ b/frontend-modern/src/components/Storage/__tests__/StoragePoolDetail.test.tsx @@ -238,6 +238,7 @@ describe('StoragePoolDetail', () => { [ { id: 'disk1', + parentId: 'storage-1', type: 'physical_disk', name: 'Disk 1', displayName: 'Disk 1', diff --git a/frontend-modern/src/components/Storage/__tests__/useStoragePoolDetailModel.test.ts b/frontend-modern/src/components/Storage/__tests__/useStoragePoolDetailModel.test.ts index 187507a5e6..9206ffef31 100644 --- a/frontend-modern/src/components/Storage/__tests__/useStoragePoolDetailModel.test.ts +++ b/frontend-modern/src/components/Storage/__tests__/useStoragePoolDetailModel.test.ts @@ -23,6 +23,7 @@ const buildRecord = (): StorageRecord => ({ metricsTarget: { resourceType: 'storage', resourceId: 'pool:tank' }, details: { node: 'truenas01', + parentId: 'host-truenas01', type: 'pool', zfsPool: { state: 'ONLINE', @@ -38,6 +39,7 @@ const buildRecord = (): StorageRecord => ({ const buildDisk = (): Resource => ({ id: 'disk-1', + parentId: 'host-truenas01', type: 'physical_disk', name: 'disk-1', displayName: 'disk-1', diff --git a/frontend-modern/src/features/storageBackups/__tests__/storagePoolDetailPresentation.branchcov.test.ts b/frontend-modern/src/features/storageBackups/__tests__/storagePoolDetailPresentation.branchcov.test.ts index ce66441060..5950ed1eef 100644 --- a/frontend-modern/src/features/storageBackups/__tests__/storagePoolDetailPresentation.branchcov.test.ts +++ b/frontend-modern/src/features/storageBackups/__tests__/storagePoolDetailPresentation.branchcov.test.ts @@ -453,16 +453,18 @@ describe('getStoragePoolLinkedDisks branch coverage', () => { id: 'cache-pool-1', name: ' My Cache ', source: unraidSource(), - details: { type: 'unraid-cache-pool', platform: 'unraid' }, + details: { type: 'unraid-cache-pool', platform: 'unraid', parentId: 'host-1' }, }); const disks = [ { id: 'in', + parentId: 'host-1', name: 'in', physicalDisk: { devPath: '/dev/sda', storageRole: 'cache', storageGroup: 'My Cache' }, }, { id: 'out', + parentId: 'host-1', name: 'out', physicalDisk: { devPath: '/dev/sdb', storageRole: 'cache', storageGroup: 'other' }, }, @@ -476,16 +478,18 @@ describe('getStoragePoolLinkedDisks branch coverage', () => { id: 'arr-1', name: 'Tower', source: unraidSource(), - details: { type: 'pool', platform: 'unraid', topology: 'array' }, + details: { type: 'pool', platform: 'unraid', topology: 'array', parentId: 'host-1' }, }); const disks = [ { id: 'in', + parentId: 'host-1', name: 'in', physicalDisk: { devPath: '/dev/sda', storageRole: 'data', storageGroup: 'unraid-array' }, }, { id: 'out', + parentId: 'host-1', name: 'out', physicalDisk: { devPath: '/dev/sdb', storageRole: 'data', storageGroup: 'nope' }, }, diff --git a/frontend-modern/src/features/storageBackups/__tests__/storagePoolDetailPresentation.test.ts b/frontend-modern/src/features/storageBackups/__tests__/storagePoolDetailPresentation.test.ts index 47f5645cfd..dc34f25a70 100644 --- a/frontend-modern/src/features/storageBackups/__tests__/storagePoolDetailPresentation.test.ts +++ b/frontend-modern/src/features/storageBackups/__tests__/storagePoolDetailPresentation.test.ts @@ -30,6 +30,7 @@ const buildRecord = (overrides: Partial = {}): StorageRecord => ( observedAt: Date.now(), details: { node: 'truenas01', + parentId: 'host-truenas01', type: 'pool', zfsPool: { state: 'ONLINE', @@ -66,6 +67,7 @@ const buildRecord = (overrides: Partial = {}): StorageRecord => ( const buildDisk = (): Resource => ({ id: 'disk-1', + parentId: 'host-truenas01', type: 'physical_disk', name: 'disk-1', displayName: 'disk-1', @@ -249,6 +251,7 @@ describe('storagePoolDetailPresentation', () => { }, details: { type: 'unraid-array', + parentId: 'host-tower', platform: 'unraid', topology: 'array', arrayState: 'STARTED', @@ -257,6 +260,7 @@ describe('storagePoolDetailPresentation', () => { const disks = [ { id: 'parity', + parentId: 'host-tower', type: 'physical_disk', name: 'Parity', displayName: 'Parity', @@ -277,6 +281,7 @@ describe('storagePoolDetailPresentation', () => { }, { id: 'disk1', + parentId: 'host-tower', type: 'physical_disk', name: 'Disk 1', displayName: 'Disk 1', diff --git a/frontend-modern/src/features/storageBackups/__tests__/storagePoolDiskOwnership.test.ts b/frontend-modern/src/features/storageBackups/__tests__/storagePoolDiskOwnership.test.ts new file mode 100644 index 0000000000..6ae92dafa8 --- /dev/null +++ b/frontend-modern/src/features/storageBackups/__tests__/storagePoolDiskOwnership.test.ts @@ -0,0 +1,170 @@ +import { describe, expect, it } from 'vitest'; +import type { State } from '@/types/api'; +import type { Resource } from '@/types/resource'; +import { buildStorageRecords } from '../storageAdapters'; +import { getStoragePoolLinkedDisks } from '../storagePoolDetailPresentation'; + +const storage = (overrides: Partial = {}): Resource => + ({ + id: 'pool-a', + type: 'storage', + name: 'tank', + displayName: 'tank', + platformId: 'provider-a', + platformType: 'proxmox-pve', + sourceType: 'api', + status: 'online', + lastSeen: 1, + parentId: 'host-a', + parentName: 'Same display name', + storage: { + type: 'zfspool', + isZfs: true, + zfsPool: { + name: 'tank', + state: 'ONLINE', + readErrors: 0, + writeErrors: 0, + checksumErrors: 0, + devices: [{ name: 'sda', type: 'disk', state: 'ONLINE' }], + }, + }, + proxmox: { nodeName: 'pve1', instance: 'cluster-a' }, + ...overrides, + }) as Resource; + +const disk = (id: string, overrides: Partial = {}): Resource => + ({ + id, + type: 'physical_disk', + name: id, + displayName: id, + platformId: 'provider-a', + platformType: 'proxmox-pve', + sourceType: 'api', + status: 'online', + lastSeen: 1, + parentId: 'host-a', + parentName: 'Same display name', + proxmox: { nodeName: 'pve1', instance: 'cluster-a' }, + physicalDisk: { devPath: '/dev/sda', model: id, temperature: 35 }, + ...overrides, + }) as Resource; + +const record = (resource = storage()) => + buildStorageRecords({ state: {} as State, resources: [resource] })[0]; +const linked = (resource: Resource, disks: Resource[]) => + getStoragePoolLinkedDisks(record(resource), disks).map((item) => item.id); + +describe('pool physical disk ownership', () => { + it('excludes other hosts and unknown owners despite identical paths and labels', () => { + expect( + linked(storage(), [ + disk('local'), + disk('remote', { parentId: 'host-b' }), + disk('unknown', { parentId: undefined, proxmox: undefined }), + ]), + ).toEqual(['local']); + }); + + it('never lets matching provider names override mismatched canonical parents', () => { + expect(linked(storage(), [disk('remote', { parentId: 'host-b' })])).toEqual([]); + }); + + it('preserves direct canonical pool and legacy resource-reference children', () => { + const target = record(); + target.refs = { resourceId: 'legacy-pool' }; + expect( + getStoragePoolLinkedDisks(target, [ + disk('direct', { parentId: 'pool-a', physicalDisk: { devPath: '/dev/other' } }), + disk('legacy', { parentId: 'legacy-pool', physicalDisk: { devPath: '/dev/legacy' } }), + ]).map((item) => item.id), + ).toEqual(['legacy', 'direct']); + }); + + it.each(['top-level', 'legacy'] as const)( + 'retains exact native instance/node ownership: %s', + (shape) => { + const pool = storage({ parentId: undefined, proxmox: undefined, platformData: undefined }); + const local = disk('local', { parentId: undefined, proxmox: undefined }); + const native = { nodeName: 'pve1', instance: 'cluster-a' }; + if (shape === 'top-level') { + pool.proxmox = native; + local.proxmox = native; + } else { + pool.platformData = { proxmox: native }; + local.platformData = { proxmox: native }; + } + expect(record(pool).details?.instance).toBe('cluster-a'); + expect( + linked(pool, [ + local, + disk('other-cluster', { + parentId: undefined, + proxmox: { ...native, instance: 'cluster-b' }, + }), + disk('other-node', { parentId: undefined, proxmox: { ...native, nodeName: 'pve2' } }), + disk('unscoped', { parentId: undefined, proxmox: { nodeName: 'pve1' } }), + ]), + ).toEqual(['local']); + }, + ); + + it('requires both native instance and node when canonical ownership is missing', () => { + expect( + linked(storage({ parentId: undefined, proxmox: { nodeName: 'pve1' } }), [ + disk('unscoped', { parentId: undefined, proxmox: { nodeName: 'pve1' } }), + ]), + ).toEqual([]); + }); + + it.each(['sda', '/dev/sda'])('matches whole device identifiers, not a suffix: %s', (name) => { + const pool = storage(); + pool.storage!.zfsPool!.devices = [ + { name, type: 'disk', state: 'ONLINE', readErrors: 0, writeErrors: 0, checksumErrors: 0 }, + ]; + expect( + linked(pool, [ + disk('local'), + disk('suffix-collision', { physicalDisk: { devPath: '/dev/not-sda' } }), + disk('partition', { physicalDisk: { devPath: '/dev/sda1' } }), + ]), + ).toEqual(['local']); + }); + + it('does not confuse nvme device numbers with a shorter vdev name', () => { + const pool = storage(); + pool.storage!.zfsPool!.devices = [ + { + name: 'n1', + type: 'disk', + state: 'ONLINE', + readErrors: 0, + writeErrors: 0, + checksumErrors: 0, + }, + ]; + expect(linked(pool, [disk('nvme', { physicalDisk: { devPath: '/dev/nvme0n1' } })])).toEqual([]); + }); + + it.each([ + ['unraid-array', 'Array', 'unraid-array'], + ['unraid-cache-pool', 'cache', 'cache'], + ])('keeps repeated UnRAID groups on their own host: %s', (type, name, group) => { + const pool = storage({ + name, + platformType: 'unraid', + proxmox: undefined, + storage: { type, platform: 'unraid' }, + }); + const member = { devPath: '/dev/sda', storageGroup: group, storageRole: 'data' }; + expect( + linked(pool, [ + disk('local', { physicalDisk: member }), + disk('remote', { parentId: 'host-b', physicalDisk: member }), + disk('unknown', { parentId: undefined, proxmox: undefined, physicalDisk: member }), + disk('different-group', { physicalDisk: { ...member, storageGroup: 'other' } }), + ]), + ).toEqual(['local']); + }); +}); diff --git a/frontend-modern/src/features/storageBackups/storageAdapters.ts b/frontend-modern/src/features/storageBackups/storageAdapters.ts index efa82a594d..19b8ac6598 100644 --- a/frontend-modern/src/features/storageBackups/storageAdapters.ts +++ b/frontend-modern/src/features/storageBackups/storageAdapters.ts @@ -224,6 +224,10 @@ const mapResourceStorageRecord = (resource: Resource, adapterId: string): Storag parentId: resource.parentId, parentName: resource.parentName, node: proxmoxNativeNode || (platformData.node as string | undefined) || storageNodes[0], + instance: + resource.proxmox?.instance || + (proxmoxPlatform?.instance as string | undefined) || + (platformData.instance as string | undefined), nodeHints, hostLabel, platformLabel, diff --git a/frontend-modern/src/features/storageBackups/storagePoolDetailPresentation.ts b/frontend-modern/src/features/storageBackups/storagePoolDetailPresentation.ts index 1fc455ff90..166571e68f 100644 --- a/frontend-modern/src/features/storageBackups/storagePoolDetailPresentation.ts +++ b/frontend-modern/src/features/storageBackups/storagePoolDetailPresentation.ts @@ -368,6 +368,28 @@ const diskRoleRank = (role: string): number => { } }; +const diskBelongsToStorageHost = (record: StorageRecord, disk: Resource): boolean => { + const hostId = readRecordDetailString(record, 'parentId'); + const diskParentId = disk.parentId?.trim(); + // Canonical ownership outranks labels, paths and source-native names. + // In particular, two hosts may both be called pve1 and both report /dev/sda. + if (hostId && diskParentId) return hostId === diskParentId; + + // Older Proxmox snapshots may lack canonical parents. Only the complete + // native instance/node pair is a scope; a hostname alone is not one. + const platformData = (disk.platformData || {}) as Record; + const proxmox = platformData.proxmox as Resource['proxmox'] | undefined; + const node = readRecordDetailString(record, 'node').toLowerCase(); + const instance = readRecordDetailString(record, 'instance'); + const diskNode = (disk.proxmox?.nodeName || disk.proxmox?.node || proxmox?.nodeName || '') + .trim() + .toLowerCase(); + const diskInstance = (disk.proxmox?.instance || proxmox?.instance || '').trim(); + return Boolean(node && instance && node === diskNode && instance === diskInstance); +}; + +const normalizeDevicePath = (value: string): string => value.trim().replace(/^\/dev\//, ''); + export function getStoragePoolLinkedDisks( record: StorageRecord, physicalDisks: Resource[], @@ -382,9 +404,14 @@ export function getStoragePoolLinkedDisks( .filter((disk) => { const devPath = readDiskDevPath(disk); const group = readDiskStorageGroup(disk).toLowerCase(); - if (directParentIds.has(disk.parentId || '')) return true; + if (directParentIds.has(disk.parentId?.trim() || '')) return true; + if (!diskBelongsToStorageHost(record, disk)) return false; if (pool?.devices?.length && devPath) { - return pool.devices.some((device) => devPath.endsWith(device.name)); + return pool.devices.some( + (device) => + Boolean(device.name.trim()) && + normalizeDevicePath(devPath) === normalizeDevicePath(device.name), + ); } return Boolean(unraidStorageGroup && group === unraidStorageGroup); }) From 7b370c7db9941bba4037a888230cb513c02927c0 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Fri, 2 Oct 2026 23:07:46 +0100 Subject: [PATCH 4/5] Make PBS History diagnostics recognise batched responses The existing drawer fetches all metrics without a metric query field. Explain metrics-object and single-metric points responses so readers do not wait for an absent CPU request or mistake missing CPU and measured zero for the same result. Contract-Neutral: Clarify existing History diagnostics and protect mirrored documentation; no runtime, API, identity, access, retention or response-shape change. Change-source: pulse-maintainer --- docs/PBS.md | 31 ++++++++++++++++++++---------- frontend-modern/public/docs/PBS.md | 31 ++++++++++++++++++++---------- scripts/tests/test_pbs_docs.py | 11 +++++++++++ 3 files changed, 53 insertions(+), 20 deletions(-) diff --git a/docs/PBS.md b/docs/PBS.md index 518298fd83..a377d2c602 100644 --- a/docs/PBS.md +++ b/docs/PBS.md @@ -242,14 +242,24 @@ Start with one affected PBS and a recent, unlocked time range: collecting the identities again. In your signed-in browser, open developer tools → **Network**, filter for `metrics-store/history`, then open the affected **History** tab. Do not refresh or restart the server. Inspect one - completed request with `metric=cpu` and its `resourceType`, `resourceId` and - `range` query fields. Do not edit or replay the request. -3. Check its HTTP status and **Response** locally. A successful JSON response - with an empty `points` array means this request returned no samples for that - target and range; it does not prove there is no data under another target. - Non-empty `points` with an empty chart points to a different display problem. - An error, a cancelled request, or an HTML sign-in page is not an empty series. - Record a redacted error if present rather than changing permissions. + completed request for the drawer's `resourceType`, `resourceId` and `range`. + The drawer normally fetches all chart metrics together: a request **without + `metric`** is expected, so do not wait for a separate `metric=cpu` request. + Do not edit or replay the request. +3. Check its HTTP status and **Response** locally. The query determines which + response field to inspect: + - **No `metric` query field:** inspect the `metrics` object. An empty + `metrics: {}` means this request returned no series. If it has a `cpu` + entry, note whether the `metrics.cpu` array is empty or non-empty; other + metrics can be present even when CPU is absent. + - **`metric=cpu`:** inspect the `points` array for that single metric. + An empty array means this request returned no CPU samples. + A successful empty result applies only to this target and range; it does + not prove there is no data under another target. Non-empty samples with an + empty chart point to a different display problem. A measured zero is a + sample, not an empty result. An error, a cancelled request, or an HTML + sign-in page is not an empty series. Record a redacted error if present + rather than changing permissions. If no matching request appears, record that fact and the visible chart message; do not paste scripts into the browser console to force one. If opening the @@ -257,8 +267,9 @@ page makes the browser unresponsive, stop and retain the observations already available. No repeated reproduction is needed. When reporting this, share only the failing route, time range, whether the -request target agrees with the drawer, HTTP status, and whether `points` is -empty or non-empty. Use consistent placeholders for private hostnames and IDs. +request target agrees with the drawer, HTTP status, and the response shape: +`metrics` empty, CPU absent, CPU empty/non-empty, or single-metric `points` +empty/non-empty. Use consistent placeholders for private hostnames and IDs. Do not share a HAR export, **Copy as cURL** output, request headers, cookies, tokens, or a full response. These checks use the existing browser session; they need no new API token or command-execution permission. diff --git a/frontend-modern/public/docs/PBS.md b/frontend-modern/public/docs/PBS.md index 518298fd83..a377d2c602 100644 --- a/frontend-modern/public/docs/PBS.md +++ b/frontend-modern/public/docs/PBS.md @@ -242,14 +242,24 @@ Start with one affected PBS and a recent, unlocked time range: collecting the identities again. In your signed-in browser, open developer tools → **Network**, filter for `metrics-store/history`, then open the affected **History** tab. Do not refresh or restart the server. Inspect one - completed request with `metric=cpu` and its `resourceType`, `resourceId` and - `range` query fields. Do not edit or replay the request. -3. Check its HTTP status and **Response** locally. A successful JSON response - with an empty `points` array means this request returned no samples for that - target and range; it does not prove there is no data under another target. - Non-empty `points` with an empty chart points to a different display problem. - An error, a cancelled request, or an HTML sign-in page is not an empty series. - Record a redacted error if present rather than changing permissions. + completed request for the drawer's `resourceType`, `resourceId` and `range`. + The drawer normally fetches all chart metrics together: a request **without + `metric`** is expected, so do not wait for a separate `metric=cpu` request. + Do not edit or replay the request. +3. Check its HTTP status and **Response** locally. The query determines which + response field to inspect: + - **No `metric` query field:** inspect the `metrics` object. An empty + `metrics: {}` means this request returned no series. If it has a `cpu` + entry, note whether the `metrics.cpu` array is empty or non-empty; other + metrics can be present even when CPU is absent. + - **`metric=cpu`:** inspect the `points` array for that single metric. + An empty array means this request returned no CPU samples. + A successful empty result applies only to this target and range; it does + not prove there is no data under another target. Non-empty samples with an + empty chart point to a different display problem. A measured zero is a + sample, not an empty result. An error, a cancelled request, or an HTML + sign-in page is not an empty series. Record a redacted error if present + rather than changing permissions. If no matching request appears, record that fact and the visible chart message; do not paste scripts into the browser console to force one. If opening the @@ -257,8 +267,9 @@ page makes the browser unresponsive, stop and retain the observations already available. No repeated reproduction is needed. When reporting this, share only the failing route, time range, whether the -request target agrees with the drawer, HTTP status, and whether `points` is -empty or non-empty. Use consistent placeholders for private hostnames and IDs. +request target agrees with the drawer, HTTP status, and the response shape: +`metrics` empty, CPU absent, CPU empty/non-empty, or single-metric `points` +empty/non-empty. Use consistent placeholders for private hostnames and IDs. Do not share a HAR export, **Copy as cURL** output, request headers, cookies, tokens, or a full response. These checks use the existing browser session; they need no new API token or command-execution permission. diff --git a/scripts/tests/test_pbs_docs.py b/scripts/tests/test_pbs_docs.py index d13c26dab5..3e9f104fcb 100644 --- a/scripts/tests/test_pbs_docs.py +++ b/scripts/tests/test_pbs_docs.py @@ -125,6 +125,17 @@ class PBSDocsTest(unittest.TestCase): "Do not delete backups", "Do not clear History"): self.assertIn(phrase, text) + def test_history_checks_cover_batched_and_single_metric_requests(self): + history = DOC.read_text().split("### PBS is connected but History stays empty\n")[1].split("\n### ")[0] + for phrase in ("without\n `metric`", "do not wait for a separate `metric=cpu` request", + "`metrics: {}`", "`metrics.cpu` array", "CPU is absent", + "`points` array for that single metric", "A measured zero is a\n sample", + "Do not edit or replay", "Do not share a HAR", "enable\nDebug logging"): + with self.subTest(phrase=phrase): + self.assertIn(phrase, history) + self.assertNotIn("completed request with `metric=cpu`", history) + self.assertIn("does\n not prove there is no data under another target", history) + def test_every_documented_shell_recipe_parses(self): for command in blocks(): with self.subTest(command=command): From f0ea29480e172631e71ed8da54de95e9cfabda22 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Fri, 2 Oct 2026 23:32:38 +0100 Subject: [PATCH 5/5] Handle exact TrueNAS subscription termination without stalled reads Recognise matching notify_unsubscribed events before and after acknowledgement, preserve unrelated streams and clean log completion, and retain only numeric errno with fixed diagnostics. Keep nonempty Apps inventory and poll-ledger progress while rejected stats recover on the next ordinary poll. The reporter supplied a source-derived envelope, not a retained capture; native acceptance and delivery remain separate. Change-source: pulse-maintainer --- .../v6/internal/subsystems/monitoring.md | 26 +++ .../v6/internal/subsystems/registry.json | 1 + internal/monitoring/truenas_poller_test.go | 127 ++++++++++++ internal/truenas/client.go | 48 ++++- internal/truenas/subscription.go | 50 +++++ internal/truenas/transport_test.go | 193 +++++++++++++++++- 6 files changed, 437 insertions(+), 8 deletions(-) create mode 100644 internal/truenas/subscription.go diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index c149fa5c1b..6265060bec 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -17,6 +17,32 @@ ## Purpose +### Exact TrueNAS subscription termination — issue #2396 + +JSON-RPC stream readers and the subscription-acknowledgement wait recognise +`notify_unsubscribed` only for the exact requested collection, including its +arguments. An unrelated interval or event source cannot reject the current +subscription. A matching rejection ends the operation promptly, discards the +stream session and is not retried or downgraded to REST. A subsequent ordinary +read may authenticate a fresh session within the existing operation budget. +The notification uses middleware errno fields, not a JSON-RPC response error; +only its numeric errno and a fixed message survive, never provider reason, +trace, extra fields or private collection arguments. Malformed termination +cannot leave a stream reusable. A clean end without telemetry remains +unavailable, not a zero sample; a clean log end returns the bounded collected +tail and retains the socket without cancelling an already-ended subscription. + +`TestJSONRPCSubscriptionTerminationRejectsWithoutWaitingOrRetry` covers all +three readers before/after acknowledgement, cleanup, confidentiality and fresh +read recovery. The adjacent termination controls pin exact-collection +isolation, malformed events, absent samples and clean log completion. +`TestTrueNASPollerSubscriptionRejectionKeepsPollingAndRecovers` exercises +three connected protocol/poller snapshots: usable inventory and poll-ledger +progress during rejected nonempty Apps enrichment, native alert disappearance, +then ordinary stats recovery with stable identity. These are source controls +using the reporter's source-derived envelope, not a retained wire capture, +native appliance/incident acceptance or release availability. + ### RAID count provenance across ingestion — issue #2369 Agent ingestion, host snapshots and canonical read-state projection retain diff --git a/docs/release-control/v6/internal/subsystems/registry.json b/docs/release-control/v6/internal/subsystems/registry.json index 222a9fd471..5b59559345 100644 --- a/docs/release-control/v6/internal/subsystems/registry.json +++ b/docs/release-control/v6/internal/subsystems/registry.json @@ -6162,6 +6162,7 @@ "internal/truenas/disk_health.go", "internal/truenas/fixtures.go", "internal/truenas/provider.go", + "internal/truenas/subscription.go", "internal/truenas/telemetry.go", "internal/truenas/testdata/core13_reporting.json", "internal/truenas/transport.go", diff --git a/internal/monitoring/truenas_poller_test.go b/internal/monitoring/truenas_poller_test.go index 9074c9cdd9..fcf3a34c68 100644 --- a/internal/monitoring/truenas_poller_test.go +++ b/internal/monitoring/truenas_poller_test.go @@ -2015,6 +2015,133 @@ func TestTrueNASPollerUnresponsiveRPCDoesNotFreezeOtherConnections(t *testing.T) } } +// #2396's nonempty Apps path: an accepted subscription is then rejected by +// middlewared. Keep the inventory, advance the poll ledger, observe native +// alert disappearance and recover stats on the next ordinary poll. This is a +// protocol/poller control, not a native appliance or incident-delivery proof. +func TestTrueNASPollerSubscriptionRejectionKeepsPollingAndRecovers(t *testing.T) { + var reject atomic.Bool + var sessions, statsSubscriptions atomic.Int32 + upgrader := websocket.Upgrader{} + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/current" { + t.Error("modern polling fell back to REST") + http.NotFound(w, r) + return + } + conn, err := upgrader.Upgrade(w, r, nil) + if err != nil { + return + } + defer conn.Close() + sessions.Add(1) + for { + var request struct { + ID int64 `json:"id"` + Method string `json:"method"` + Params []json.RawMessage `json:"params"` + } + if conn.ReadJSON(&request) != nil { + return + } + var result any = []any{} + var collection string + switch request.Method { + case "auth.login_ex": + result = map[string]any{"response_type": "SUCCESS"} + case "system.info": + result = map[string]any{"hostname": "reject-nas", "version": "TrueNAS-SCALE-25.04.2.6", "system_serial": "REJECT-FIXTURE"} + case "pool.query": + result = []map[string]any{{"id": 1, "name": "tank", "status": "ONLINE", "size": 1000, "allocated": 400}} + case "app.query": + result = []map[string]any{{"id": "fixture-app", "name": "fixture-app", "state": "RUNNING"}} + case "alert.list": + if !reject.Load() { + result = []map[string]any{{"id": "fixture-alert", "level": "WARNING", "formatted": "fixture warning", "source": "fixture"}} + } + case "core.subscribe": + if len(request.Params) != 1 || json.Unmarshal(request.Params[0], &collection) != nil { + t.Error("invalid subscription") + return + } + result = "fixture-sub" + case "core.unsubscribe": + result = nil + } + if conn.WriteJSON(map[string]any{"jsonrpc": "2.0", "id": request.ID, "result": result}) != nil { + return + } + if collection == "" { + continue + } + var params any + method := "collection_update" + if strings.HasPrefix(collection, "app.stats:") { + statsSubscriptions.Add(1) + if reject.Load() { + method = "notify_unsubscribed" + params = map[string]any{"collection": collection, "error": map[string]any{"error": 14, "errname": "EFAULT", "reason": "[EFAULT] Apps are not available", "trace": nil, "extra": nil}} + } else { + params = map[string]any{"collection": collection, "fields": []any{map[string]any{"app_name": "fixture-app", "cpu_usage": 12}}} + } + } else { + params = map[string]any{"collection": collection, "fields": map[string]any{"cpu": map[string]any{"usage": 10}}} + } + if conn.WriteJSON(map[string]any{"jsonrpc": "2.0", "method": method, "params": params}) != nil { + return + } + } + })) + t.Cleanup(server.Close) + client, err := truenas.NewClient(truenas.ClientConfig{ + Host: server.URL, APIKey: "fixture-key", Username: "fixture-user", Timeout: 2 * time.Second, + InsecureSkipVerify: true, Fingerprint: fmt.Sprintf("%x", sha256.Sum256(server.Certificate().Raw)), + }) + if err != nil { + t.Fatal(err) + } + t.Cleanup(client.Close) + instance := config.TrueNASInstance{ID: "reject-connection", Host: server.URL, Enabled: true} + provider := truenas.NewLiveProviderForConnection(&truenas.APIFetcher{Client: client}, instance.ID) + poller := NewTrueNASPoller(nil, 0, nil) + poller.providersByOrg["default"] = map[string]*truenas.Provider{instance.ID: provider} + poller.configsByOrg["default"] = map[string]config.TrueNASInstance{instance.ID: instance} + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + var lastSuccess time.Time + for poll := 0; poll < 3; poll++ { + reject.Store(poll == 1) + poller.ensureConnectionRuntimeStatusLocked("default", instance.ID).nextPollAt = time.Now().Add(-time.Second) + started := time.Now() + poller.pollAll(ctx) + if elapsed := time.Since(started); elapsed >= time.Second { + t.Fatalf("rejection stalled poll %d for %s", poll, elapsed) + } + summary := poller.ConnectionSummaries("default", []config.TrueNASInstance{instance})[instance.ID] + if summary.Poll.LastError != nil || summary.Poll.LastSuccessAt == nil || !summary.Poll.LastSuccessAt.After(lastSuccess) || summary.Poll.LastAttemptAt == nil { + t.Fatalf("optional stats rejection prevented poll progress: %+v", summary) + } + lastSuccess = *summary.Poll.LastSuccessAt + snapshot := provider.Snapshot() + if snapshot == nil || snapshot.System.Hostname != "reject-nas" || len(snapshot.Pools) != 1 || len(snapshot.Apps) != 1 || snapshot.System.CPUPercent != 10 { + t.Fatalf("poll %d lost usable inventory/telemetry: %+v", poll, snapshot) + } + if poll == 1 { + if snapshot.Apps[0].Stats != nil || len(snapshot.Alerts) != 0 { + t.Fatal("rejected stats fabricated data or left the former native alert") + } + } else if snapshot.Apps[0].Stats == nil || snapshot.Apps[0].Stats.CPUPercent != 12 || len(snapshot.Alerts) != 1 { + t.Fatal("healthy stats/native alerts did not return") + } + if !hasTrueNASHostForOrg(poller, "default", "reject-nas") { + t.Fatal("poll lost the appliance identity") + } + } + if statsSubscriptions.Load() != 3 || sessions.Load() != 2 { + t.Fatalf("rejection replayed or churned later polls: subscriptions=%d sessions=%d", statsSubscriptions.Load(), sessions.Load()) + } +} + type trueNASMockServer struct { server *httptest.Server requests atomic.Int64 diff --git a/internal/truenas/client.go b/internal/truenas/client.go index ada44b00fb..b057e40d18 100644 --- a/internal/truenas/client.go +++ b/internal/truenas/client.go @@ -258,7 +258,7 @@ func (c *Client) GetSystemTelemetry(ctx context.Context) (*SystemInfo, error) { if err != nil { return err } - telemetry, err = rpc.readSystemTelemetryEvent(ctx, defaultRealtimeIntervalSeconds) + telemetry, err = rpc.readSystemTelemetryEvent(ctx, defaultRealtimeIntervalSeconds, subscriptionName) if err != nil { return discardRPCSessionForStreamError("reporting.realtime", err) } @@ -1817,7 +1817,7 @@ func (c *Client) GetAppStats(ctx context.Context) (map[string]AppStats, error) { if err != nil { return err } - stats, err = rpc.readAppStatsEvent(ctx, defaultAppStatsIntervalSeconds) + stats, err = rpc.readAppStatsEvent(ctx, defaultAppStatsIntervalSeconds, subscriptionName) if err != nil { return discardRPCSessionForStreamError("app.stats", err) } @@ -1873,7 +1873,10 @@ func (c *Client) GetAppLogs(ctx context.Context, appName, containerID string, ta return err } var reusable bool - lines, reusable, err = rpc.readAppLogEvents(ctx, tailLines) + lines, reusable, err = rpc.readAppLogEvents(ctx, tailLines, subscriptionName) + if errors.Is(err, errRPCSubscriptionComplete) { + return nil + } if err != nil { return discardRPCSessionForStreamError("app.container_log_follow", err) } @@ -2656,6 +2659,21 @@ func (c *trueNASRPCClient) call(ctx context.Context, method string, params any, return &RPCTransportError{Method: method, Phase: "read", Err: err} } if message.Method != "" { + // A rejection can race with the subscription acknowledgement. + // Do not drop it while waiting for the core.subscribe response. + if method == "core.subscribe" { + if args, ok := params.([]any); ok && len(args) == 1 { + if collection, ok := args[0].(string); ok { + ended, err := rpcSubscriptionTermination(message, collection, method) + if ended || err != nil { + if err == nil { + err = rpcSubscriptionEndedBeforeData(method) + } + return discardRPCSessionForStreamError(method, err) + } + } + } + } continue } if message.ID != request.ID { @@ -2674,7 +2692,7 @@ func (c *trueNASRPCClient) call(ctx context.Context, method string, params any, } } -func (c *trueNASRPCClient) readAppStatsEvent(ctx context.Context, intervalSeconds int) (map[string]AppStats, error) { +func (c *trueNASRPCClient) readAppStatsEvent(ctx context.Context, intervalSeconds int, collection string) (map[string]AppStats, error) { if c == nil || c.conn == nil { return nil, fmt.Errorf("truenas rpc connection is nil") } @@ -2690,6 +2708,12 @@ func (c *trueNASRPCClient) readAppStatsEvent(ctx context.Context, intervalSecond if err := c.conn.ReadJSON(&message); err != nil { return nil, &RPCTransportError{Method: "app.stats", Phase: "read", Err: err} } + if ended, err := rpcSubscriptionTermination(message, collection, "app.stats"); ended || err != nil { + if err == nil { + err = rpcSubscriptionEndedBeforeData("app.stats") + } + return nil, err + } if message.Method == "" { if message.Error != nil { return nil, rpcErrorFromWire("app.stats", message.Error) @@ -2742,7 +2766,7 @@ func (c *trueNASRPCClient) readAppStatsEvent(ctx context.Context, intervalSecond } } -func (c *trueNASRPCClient) readAppLogEvents(ctx context.Context, tailLines int) ([]AppLogLine, bool, error) { +func (c *trueNASRPCClient) readAppLogEvents(ctx context.Context, tailLines int, collection string) ([]AppLogLine, bool, error) { if c == nil || c.conn == nil { return nil, false, fmt.Errorf("truenas rpc connection is nil") } @@ -2788,6 +2812,12 @@ func (c *trueNASRPCClient) readAppLogEvents(ctx context.Context, tailLines int) } return nil, false, &RPCTransportError{Method: "app.container_log_follow", Phase: "read", Err: err} } + if ended, err := rpcSubscriptionTermination(message, collection, "app.container_log_follow"); ended || err != nil { + if err != nil { + return nil, false, err + } + return trimAppLogLines(lines, tailLines), true, errRPCSubscriptionComplete + } if message.Method == "" { if message.Error != nil { return nil, false, rpcErrorFromWire("app.container_log_follow", message.Error) @@ -3096,7 +3126,7 @@ func (c *trueNASRPCClient) getReportingDataWithQuery(ctx context.Context, graphs return response, nil } -func (c *trueNASRPCClient) readSystemTelemetryEvent(ctx context.Context, intervalSeconds int) (*SystemInfo, error) { +func (c *trueNASRPCClient) readSystemTelemetryEvent(ctx context.Context, intervalSeconds int, collection string) (*SystemInfo, error) { if c == nil || c.conn == nil { return nil, fmt.Errorf("truenas rpc connection is nil") } @@ -3112,6 +3142,12 @@ func (c *trueNASRPCClient) readSystemTelemetryEvent(ctx context.Context, interva if err := c.conn.ReadJSON(&message); err != nil { return nil, &RPCTransportError{Method: "reporting.realtime", Phase: "read", Err: err} } + if ended, err := rpcSubscriptionTermination(message, collection, "reporting.realtime"); ended || err != nil { + if err == nil { + err = rpcSubscriptionEndedBeforeData("reporting.realtime") + } + return nil, err + } if message.Method == "" { if message.Error != nil { return nil, rpcErrorFromWire("reporting.realtime", message.Error) diff --git a/internal/truenas/subscription.go b/internal/truenas/subscription.go new file mode 100644 index 0000000000..cd60b1864d --- /dev/null +++ b/internal/truenas/subscription.go @@ -0,0 +1,50 @@ +package truenas + +import ( + "encoding/json" + "errors" + "fmt" +) + +// A clean terminal event completes a log tail without leaving a subscription +// to cancel. It is not a sample for a telemetry stream. +var errRPCSubscriptionComplete = errors.New("truenas rpc subscription completed") + +// rpcSubscriptionTermination recognises only the exact collection we subscribed +// to, including its arguments. Other subscriptions (even another interval for +// the same event source) must not terminate the current reader. middlewared's +// notification error is not a JSON-RPC response error: it uses errno/reason +// fields, and may contain private names and traces. Retain only its numeric +// errno and a fixed message, never that wire text or the collection arguments. +func rpcSubscriptionTermination(message trueNASRPCResponse, collection, method string) (bool, error) { + if message.Method != "notify_unsubscribed" { + return false, nil + } + var notification struct { + Collection string `json:"collection"` + Error json.RawMessage `json:"error"` + } + if err := json.Unmarshal(message.Params, ¬ification); err != nil { + return false, fmt.Errorf("invalid truenas %s termination notification", method) + } + if notification.Collection != collection { + return false, nil + } + if len(notification.Error) == 0 { + return true, fmt.Errorf("invalid truenas %s termination notification", method) + } + if string(notification.Error) == "null" { + return true, nil + } + var failure struct { + Errno int `json:"error"` + } + if err := json.Unmarshal(notification.Error, &failure); err != nil { + return true, fmt.Errorf("invalid truenas %s termination error", method) + } + return true, &RPCError{Method: method, Code: failure.Errno, Message: "subscription rejected"} +} + +func rpcSubscriptionEndedBeforeData(method string) error { + return &RPCError{Method: method, Message: "subscription ended before a sample"} +} diff --git a/internal/truenas/transport_test.go b/internal/truenas/transport_test.go index cdbf583517..4dc03527f5 100644 --- a/internal/truenas/transport_test.go +++ b/internal/truenas/transport_test.go @@ -21,10 +21,192 @@ import ( "github.com/rs/zerolog/log" ) +// Subscription terminal-message regressions for #2396. The envelope is from +// the reporter's source-derived 25.04.2.6 example, not a retained wire capture. +func TestJSONRPCSubscriptionTerminationRejectsWithoutWaitingOrRetry(t *testing.T) { + for _, stream := range []string{"app.stats", "reporting.realtime", "app.container_log_follow"} { + for _, beforeReply := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/before_reply=%t", stream, beforeReply), func(t *testing.T) { + var subscribes, unsubscribes atomic.Int32 + fixture := newProtocolFixture(t, func(_ int, request trueNASRPCRequest) protocolFixtureReply { + switch request.Method { + case "auth.login_ex": + return protocolFixtureReply{result: map[string]any{"response_type": "SUCCESS"}} + case "reporting.get_data": + return protocolFixtureReply{result: []any{}} + case "core.subscribe": + subscribes.Add(1) + collection := request.Params.([]any)[0].(string) + return protocolFixtureReply{result: "private-sub-id", beforeReply: beforeReply, notifications: []protocolFixtureNotification{{ + method: "notify_unsubscribed", params: map[string]any{"collection": collection, "error": map[string]any{ + "error": 14, "errname": "EFAULT", "reason": "private-app-name fixture-key fixture-user https://private.invalid", + "trace": "private-trace", "extra": "private-extra", + }}, + }}} + case "core.unsubscribe": + unsubscribes.Add(1) + return protocolFixtureReply{result: nil} + case "pool.query": + return protocolFixtureReply{result: []any{}} + default: + t.Errorf("unexpected method %s", request.Method) + return protocolFixtureReply{close: true} + } + }, nil) + client := protocolFixtureClient(t, fixture.server.URL, ClientConfig{APIKey: "fixture-key", Username: "fixture-user", Timeout: 2 * time.Second}) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + started := time.Now() + var err error + switch stream { + case "app.stats": + var stats map[string]AppStats + stats, err = client.GetAppStats(ctx) + if stats != nil { + t.Error("rejected stats fabricated a sample") + } + case "reporting.realtime": + var telemetry *SystemInfo + telemetry, err = client.GetSystemTelemetry(ctx) + if telemetry != nil { + t.Error("rejected telemetry fabricated a sample") + } + default: + var lines []AppLogLine + lines, err = client.GetAppLogs(ctx, "private-app-name", "private-container", 100) + if lines != nil { + t.Error("rejected logs appeared successful") + } + } + var rpcErr *RPCError + if !errors.As(err, &rpcErr) || rpcErr.Code != 14 || rpcErr.Message != "subscription rejected" { + t.Fatalf("termination = %v, want structured errno=14 rejection", err) + } + if elapsed := time.Since(started); elapsed >= time.Second { + t.Fatalf("terminal event waited for the operation budget: %s", elapsed) + } + for _, private := range []string{"private-app-name", "fixture-key", "fixture-user", "private.invalid", "private-trace", "private-extra", "private-sub-id", "private-container"} { + if strings.Contains(err.Error(), private) || strings.Contains(client.TransportStatus().LastError, private) { + t.Fatalf("termination exposed %s", private) + } + } + if client.rpc != nil || client.TransportStatus().Connected || subscribes.Load() != 1 || unsubscribes.Load() != 0 || fixture.sessions.Load() != 1 { + t.Fatal("terminal rejection retried, reused the stream or tried to cancel its ended subscription") + } + if _, err := client.GetPools(ctx); err != nil || fixture.sessions.Load() != 2 || fixture.restRequests.Load() != 0 { + t.Fatalf("next read did not recover on a fresh RPC session: %v", err) + } + }) + } + } +} + +func TestJSONRPCSubscriptionTerminationIgnoresOtherCollections(t *testing.T) { + var unsubscribes atomic.Int32 + fixture := newProtocolFixture(t, func(_ int, request trueNASRPCRequest) protocolFixtureReply { + switch request.Method { + case "auth.login_ex": + return protocolFixtureReply{result: map[string]any{"response_type": "SUCCESS"}} + case "core.subscribe": + collection := request.Params.([]any)[0].(string) + return protocolFixtureReply{result: "stats-sub", notifications: []protocolFixtureNotification{ + {method: "notify_unsubscribed", params: map[string]any{"collection": "app.stats:{\"interval\":99}", "error": map[string]any{"error": 14}}}, + {method: "notify_unsubscribed", params: map[string]any{"collection": "app.stats.extra:{\"interval\":2}", "error": "malformed-unrelated-error"}}, + {method: "notify_unsubscribed", params: map[string]any{"collection": "reporting.realtime:{\"interval\":2}", "error": nil}}, + {method: "collection_update", params: map[string]any{"collection": collection, "fields": []any{map[string]any{"app_name": "fixture-app", "cpu_usage": 7}}}}, + }} + case "core.unsubscribe": + unsubscribes.Add(1) + return protocolFixtureReply{result: nil} + default: + return protocolFixtureReply{result: []any{}} + } + }, nil) + client := protocolFixtureClient(t, fixture.server.URL, ClientConfig{APIKey: "fixture-key", Username: "fixture-user"}) + for poll := 0; poll < 2; poll++ { + stats, err := client.GetAppStats(context.Background()) + if err != nil || stats["fixture-app"].CPUPercent != 7 { + t.Fatalf("unrelated terminal event interrupted stats: %v, %+v", err, stats) + } + } + if fixture.sessions.Load() != 1 || unsubscribes.Load() != 2 || !client.TransportStatus().Connected { + t.Fatal("unrelated event prevented ordinary cleanup and session reuse") + } +} + +func TestJSONRPCSubscriptionCleanTerminationDoesNotInventStats(t *testing.T) { + fixture := newProtocolFixture(t, func(_ int, request trueNASRPCRequest) protocolFixtureReply { + if request.Method == "auth.login_ex" { + return protocolFixtureReply{result: map[string]any{"response_type": "SUCCESS"}} + } + if request.Method == "core.subscribe" { + return protocolFixtureReply{result: "stats-sub", notifications: []protocolFixtureNotification{{method: "notify_unsubscribed", params: map[string]any{"collection": request.Params.([]any)[0], "error": nil}}}} + } + return protocolFixtureReply{result: []any{}} + }, nil) + client := protocolFixtureClient(t, fixture.server.URL, ClientConfig{APIKey: "fixture-key", Username: "fixture-user", Timeout: 2 * time.Second}) + stats, err := client.GetAppStats(context.Background()) + var rpcErr *RPCError + if stats != nil || !errors.As(err, &rpcErr) || rpcErr.Message != "subscription ended before a sample" || client.TransportStatus().Connected { + t.Fatalf("clean termination appeared sampled or left an unresolved stream: %v, %+v", err, stats) + } +} + +func TestJSONRPCSubscriptionCleanLogEndKeepsSession(t *testing.T) { + var unsubscribes atomic.Int32 + fixture := newProtocolFixture(t, func(_ int, request trueNASRPCRequest) protocolFixtureReply { + if request.Method == "auth.login_ex" { + return protocolFixtureReply{result: map[string]any{"response_type": "SUCCESS"}} + } + if request.Method == "core.subscribe" { + collection := request.Params.([]any)[0] + return protocolFixtureReply{result: "log-sub", notifications: []protocolFixtureNotification{ + {method: "collection_update", params: map[string]any{"collection": collection, "fields": []any{map[string]any{"data": "fixture-line"}}}}, + {method: "notify_unsubscribed", params: map[string]any{"collection": "app.container_log_follow:{}", "error": map[string]any{"error": 14}}}, + {method: "notify_unsubscribed", params: map[string]any{"collection": collection, "error": nil}}, + }} + } + if request.Method == "core.unsubscribe" { + unsubscribes.Add(1) + } + return protocolFixtureReply{result: []any{}} + }, nil) + client := protocolFixtureClient(t, fixture.server.URL, ClientConfig{APIKey: "fixture-key", Username: "fixture-user"}) + lines, err := client.GetAppLogs(context.Background(), "fixture-app", "fixture-container", 100) + if err != nil || len(lines) != 1 || lines[0].Data != "fixture-line" { + t.Fatalf("clean log end lost the bounded tail: %v, %+v", err, lines) + } + if _, err := client.GetPools(context.Background()); err != nil || fixture.sessions.Load() != 1 || unsubscribes.Load() != 0 || !client.TransportStatus().Connected { + t.Fatalf("clean log end discarded or unsubscribed an already-ended stream: %v", err) + } +} + +func TestJSONRPCSubscriptionMalformedTerminalDiscardsWithoutWireText(t *testing.T) { + for _, terminal := range []any{map[string]any{"error": "private-invalid-errno", "reason": "private-error"}, "private-error", true} { + t.Run(fmt.Sprintf("%T", terminal), func(t *testing.T) { + fixture := newProtocolFixture(t, func(_ int, request trueNASRPCRequest) protocolFixtureReply { + if request.Method == "auth.login_ex" { + return protocolFixtureReply{result: map[string]any{"response_type": "SUCCESS"}} + } + if request.Method == "core.subscribe" { + return protocolFixtureReply{result: "stats-sub", notifications: []protocolFixtureNotification{{method: "notify_unsubscribed", params: map[string]any{"collection": request.Params.([]any)[0], "error": terminal}}}} + } + return protocolFixtureReply{result: []any{}} + }, nil) + client := protocolFixtureClient(t, fixture.server.URL, ClientConfig{APIKey: "fixture-key", Username: "fixture-user", Timeout: 2 * time.Second}) + _, err := client.GetAppStats(context.Background()) + if err == nil || strings.Contains(err.Error(), "private-") || !strings.Contains(err.Error(), "invalid truenas app.stats termination error") || client.rpc != nil || fixture.sessions.Load() != 1 { + t.Fatalf("malformed termination was lost, leaked or reused: %v", err) + } + }) + } +} + type protocolFixtureReply struct { result any err *trueNASRPCError notifications []protocolFixtureNotification + beforeReply bool close bool } @@ -89,8 +271,10 @@ func newProtocolFixture( } response.Result = raw } - if err := conn.WriteJSON(response); err != nil { - return + if !reply.beforeReply { + if err := conn.WriteJSON(response); err != nil { + return + } } for _, notification := range reply.notifications { params, err := json.Marshal(notification.params) @@ -106,6 +290,11 @@ func newProtocolFixture( return } } + if reply.beforeReply { + if err := conn.WriteJSON(response); err != nil { + return + } + } } })) t.Cleanup(fixture.server.Close)