From 0308490aef73944c0092e022edda271ba5b153a7 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 16 May 2026 16:36:39 -0400 Subject: [PATCH] mbproxy: close out the dashboard code-review minor findings Resolves the remaining Minor items from the 2026-05-15 review so the web-UI dashboard work has no open follow-ups: a real-HubConnection end-to-end test for the SignalR feed, stable mbproxy.admin.broadcast.* log-event names, keyboard/aria accessibility on the fleet table, frontend JS hardening (URL-decode guard, NaN guards, shared util.js), reconciler<->capture-registry coverage, throwing-sink and embedded-asset tests, broadcaster polish, and a soft upper bound on AdminPushIntervalMs. Co-Authored-By: Claude Opus 4.7 (1M context) --- mbproxy/docs/Operations/Configuration.md | 4 +- mbproxy/docs/Operations/StatusPage.md | 2 +- mbproxy/docs/Reference/LogEvents.md | 44 ++++- .../src/Mbproxy/Admin/StatusBroadcaster.cs | 56 +++++- .../src/Mbproxy/Admin/wwwroot/dashboard.css | 14 +- .../src/Mbproxy/Admin/wwwroot/dashboard.js | 53 +++--- mbproxy/src/Mbproxy/Admin/wwwroot/detail.js | 27 ++- mbproxy/src/Mbproxy/Admin/wwwroot/index.html | 29 ++-- mbproxy/src/Mbproxy/Admin/wwwroot/plc.html | 3 +- mbproxy/src/Mbproxy/Admin/wwwroot/util.js | 20 +++ .../Mbproxy/Configuration/ReloadValidator.cs | 5 +- mbproxy/src/Mbproxy/Options/MbproxyOptions.cs | 11 +- .../Mbproxy.Tests/Admin/AdminEndpointTests.cs | 17 +- .../Admin/EmbeddedAssetsTests.cs | 63 +++++++ .../Mbproxy.Tests/Admin/HubStatusE2ETests.cs | 160 ++++++++++++++++++ .../tests/Mbproxy.Tests/Admin/SignalRFakes.cs | 15 ++ .../Admin/StatusBroadcasterTests.cs | 29 +++- .../Configuration/ConfigReconcilerTests.cs | 52 +++++- .../Configuration/ReloadValidatorTests.cs | 31 ++++ .../tests/Mbproxy.Tests/Mbproxy.Tests.csproj | 3 + mbproxy/tests/sim/mbproxy.smoke.config.json | 5 +- 21 files changed, 576 insertions(+), 67 deletions(-) create mode 100644 mbproxy/src/Mbproxy/Admin/wwwroot/util.js create mode 100644 mbproxy/tests/Mbproxy.Tests/Admin/EmbeddedAssetsTests.cs create mode 100644 mbproxy/tests/Mbproxy.Tests/Admin/HubStatusE2ETests.cs diff --git a/mbproxy/docs/Operations/Configuration.md b/mbproxy/docs/Operations/Configuration.md index d83e577..69fcd0d 100644 --- a/mbproxy/docs/Operations/Configuration.md +++ b/mbproxy/docs/Operations/Configuration.md @@ -119,9 +119,9 @@ Server-push cadence (milliseconds) for the admin dashboard's SignalR feed. Every | Field | Type | Default | Range | |-------|------|---------|-------| -| `AdminPushIntervalMs` | int | `1000` | `> 0` | +| `AdminPushIntervalMs` | int | `1000` | `1`–`60000` | -`MbproxyOptionsValidator` and `ReloadValidator` both reject values `<= 0`. The broadcaster additionally floors the effective interval at 100 ms. Source: `MbproxyOptions.AdminPushIntervalMs`. +`MbproxyOptionsValidator` and `ReloadValidator` both reject values outside `1`–`60000` ms — the upper bound is a soft guard against a typo (e.g. a seconds value pasted as milliseconds) that would make the "live" feed effectively non-live. The broadcaster additionally floors the effective interval at 100 ms. Source: `MbproxyOptions.AdminPushIntervalMs`. ## `Mbproxy.Plcs[]` diff --git a/mbproxy/docs/Operations/StatusPage.md b/mbproxy/docs/Operations/StatusPage.md index 9e6a02d..d4ae04a 100644 --- a/mbproxy/docs/Operations/StatusPage.md +++ b/mbproxy/docs/Operations/StatusPage.md @@ -302,7 +302,7 @@ The UI is a Bootstrap 5 single-page app served from embedded assets under `src/M 1. **App bar** — service version, formatted uptime, accepted-reload count, and a live SignalR connection-state pill. 2. **Aggregate strip** — six cards: listeners bound/configured, total connected clients, fleet PDU/s (rate derived client-side from successive snapshots), PLCs in `recovering`, total backend exceptions, fleet cache hit ratio. The recovering / exceptions cards highlight when non-zero. -3. **KPI table** — one row per configured PLC, Tier-1 columns only: PLC name, backend `host:listenPort`, state chip (`bound` green / `recovering` amber / `stopped` grey), clients, PDU/s, RTT ms, exception total, coalesce %, cache %, keepalive. The table is client-side filterable (name/host search, state, "problems only") and sortable. Clicking a row opens that PLC's detail page in a new tab. +3. **KPI table** — one row per configured PLC, Tier-1 columns only: PLC name, backend `host:listenPort`, state chip (`bound` green / `recovering` amber / `stopped` grey), clients, PDU/s, RTT ms, exception total, coalesce %, cache %, keepalive. The table is client-side filterable (name/host search, state, "problems only") and sortable — column headers are keyboard-operable (Tab to focus, Enter/Space to sort) and carry `aria-sort`. The PLC name is a link that opens that PLC's detail page in a new tab. ### Connection detail (`GET /plc/{name}`) diff --git a/mbproxy/docs/Reference/LogEvents.md b/mbproxy/docs/Reference/LogEvents.md index 7840073..c63a783 100644 --- a/mbproxy/docs/Reference/LogEvents.md +++ b/mbproxy/docs/Reference/LogEvents.md @@ -138,6 +138,48 @@ Fires when the admin endpoint cannot bind its configured `AdminPort`. The servic **Operator action:** change `Mbproxy:AdminPort` in `appsettings.json` to a free port. Hot-reload picks up the change; the admin endpoint rebinds without a service restart. +### mbproxy.admin.broadcast.snapshot.failed + +**Level:** Error · **EventId:** 72 · **Source:** `src/Mbproxy/Admin/StatusBroadcaster.cs` + +No structured properties; the exception is attached. + +Fires when the live-dashboard push loop cannot build a status snapshot. The current push cycle is skipped; the loop retries on the next interval. The proxy data path is unaffected. + +**Operator action:** none if isolated. A sustained rate means the status-snapshot builder is consistently throwing — capture the attached exception and investigate. + +### mbproxy.admin.broadcast.fleet.failed + +**Level:** Error · **EventId:** 73 · **Source:** `src/Mbproxy/Admin/StatusBroadcaster.cs` + +No structured properties; the exception is attached. + +Fires when the push loop fails to deliver the fleet snapshot to dashboard subscribers (a SignalR transport fault). The loop continues; per-PLC detail pushes are still attempted. + +**Operator action:** none if isolated. Sustained occurrences mean the SignalR feed is unhealthy — the dashboard's "live" feed is stale even though the proxy is fine. + +### mbproxy.admin.broadcast.detail.failed + +**Level:** Error · **EventId:** 74 · **Source:** `src/Mbproxy/Admin/StatusBroadcaster.cs` + +| Property | Type | Meaning | +|----------|------|---------| +| `Plc` | `string` | Configured PLC name whose detail push failed. | + +Fires when the push loop fails to deliver a per-PLC detail snapshot to that PLC's detail-page subscribers. The loop continues with the remaining PLCs. + +**Operator action:** none if isolated. Sustained occurrences for one `Plc` mean that PLC's detail page is not receiving live updates. + +### mbproxy.admin.broadcast.loop.terminated + +**Level:** Error · **EventId:** 75 · **Source:** `src/Mbproxy/Admin/StatusBroadcaster.cs` + +No structured properties; the exception is attached. + +Fires when the live-dashboard push loop itself terminates on an unhandled exception (not the expected cancellation at shutdown). The dashboard's live feed stops entirely until the admin endpoint is rebound (an `AdminPort` hot-reload restarts the loop). + +**Operator action:** alert. The live feed is dead; capture the attached exception and restart the admin endpoint (toggle `Mbproxy:AdminPort`) or the service. + ### mbproxy.shutdown.complete **Level:** Information · **EventId:** 80 · **Source:** `src/Mbproxy/Diagnostics/ShutdownCoordinator.cs` @@ -531,7 +573,7 @@ Each subsystem owns a single `*LogEvents.cs` static partial class with `[LoggerM - `src/Mbproxy/Proxy/Cache/CacheLogEvents.cs` — response cache. - `src/Mbproxy/Proxy/RewriterLogEvents.cs` — BCD rewriting and exception passthrough. -Lifecycle events (`startup.*`, `listener.*`, `admin.*`, `shutdown.*`, `config.reload.*`) live as private `[LoggerMessage]` declarations next to the class that emits them — see `ProxyWorker.cs`, `PlcListener.cs`, `PlcListenerSupervisor.cs`, `AdminEndpointHost.cs`, `ShutdownCoordinator.cs`, and `ConfigReconciler.cs`. New subsystems should follow the `*LogEvents.cs` pattern when they accumulate more than two events. +Lifecycle events (`startup.*`, `listener.*`, `admin.*`, `shutdown.*`, `config.reload.*`) live as private `[LoggerMessage]` declarations next to the class that emits them — see `ProxyWorker.cs`, `PlcListener.cs`, `PlcListenerSupervisor.cs`, `AdminEndpointHost.cs`, `StatusBroadcaster.cs` (the `admin.broadcast.*` family), `ShutdownCoordinator.cs`, and `ConfigReconciler.cs`. New subsystems should follow the `*LogEvents.cs` pattern when they accumulate more than two events. ## Related Documentation diff --git a/mbproxy/src/Mbproxy/Admin/StatusBroadcaster.cs b/mbproxy/src/Mbproxy/Admin/StatusBroadcaster.cs index e60cf98..ca9a403 100644 --- a/mbproxy/src/Mbproxy/Admin/StatusBroadcaster.cs +++ b/mbproxy/src/Mbproxy/Admin/StatusBroadcaster.cs @@ -20,7 +20,7 @@ namespace Mbproxy.Admin; /// SignalR host and all connections without firing per-connection disconnect cleanup /// deterministically — never leaves a capture armed with no viewer. /// -internal sealed class StatusBroadcaster : IAsyncDisposable +internal sealed partial class StatusBroadcaster : IAsyncDisposable { private readonly IStatusPushSink _sink; private readonly StatusSnapshotBuilder _builder; @@ -32,6 +32,11 @@ internal sealed class StatusBroadcaster : IAsyncDisposable private readonly CancellationTokenSource _cts = new(); private Task _loop = Task.CompletedTask; + // Guards StopAsync against a double-stop (DisposeAsync also calls StopAsync, and the + // owner may call StopAsync explicitly first) — symmetry with AdminEndpointHost's + // _disposed flag, and defends a future caller from touching the disposed CTS. + private bool _stopped; + public StatusBroadcaster( IStatusPushSink sink, StatusSnapshotBuilder builder, @@ -56,6 +61,9 @@ internal sealed class StatusBroadcaster : IAsyncDisposable /// public async Task StopAsync() { + if (_stopped) return; + _stopped = true; + if (!_cts.IsCancellationRequested) await _cts.CancelAsync().ConfigureAwait(false); @@ -81,7 +89,7 @@ internal sealed class StatusBroadcaster : IAsyncDisposable } catch (Exception ex) { - _logger.LogError(ex, "StatusBroadcaster: failed to build status snapshot"); + LogSnapshotFailed(_logger, ex); return; } @@ -91,7 +99,7 @@ internal sealed class StatusBroadcaster : IAsyncDisposable } catch (Exception ex) when (ex is not OperationCanceledException) { - _logger.LogError(ex, "StatusBroadcaster: fleet push failed"); + LogFleetPushFailed(_logger, ex); } // Reconcile capture arm state from the live viewer set. This is the single @@ -100,18 +108,24 @@ internal sealed class StatusBroadcaster : IAsyncDisposable var activePlcs = _tracker.ActivePlcs(); _captureRegistry.ReconcileArmed(activePlcs); + // Index the snapshot's PLC rows once per cycle — a per-active-PLC FirstOrDefault + // would be O(active × fleet). + Dictionary? plcsByName = activePlcs.Count > 0 + ? snapshot.Plcs.ToDictionary(p => p.Name, StringComparer.Ordinal) + : null; + foreach (var plcName in activePlcs) { try { - var plc = snapshot.Plcs.FirstOrDefault(p => p.Name == plcName); + var plc = plcsByName!.GetValueOrDefault(plcName); var debug = _builder.BuildDebug(plcName); var detail = new PlcDetailResponse(plc, debug); await _sink.PushPlcAsync(plcName, detail, ct).ConfigureAwait(false); } catch (Exception ex) when (ex is not OperationCanceledException) { - _logger.LogError(ex, "StatusBroadcaster: detail push failed for PLC {Plc}", plcName); + LogDetailPushFailed(_logger, plcName, ex); } } } @@ -122,12 +136,15 @@ internal sealed class StatusBroadcaster : IAsyncDisposable { while (!ct.IsCancellationRequested) { + // Push first, delay second — so a dashboard that connects right after the + // loop starts gets a snapshot immediately instead of waiting one interval. + await PushOnceAsync(ct).ConfigureAwait(false); + // Re-read the interval each cycle so an AdminPushIntervalMs hot-reload // takes effect without restarting the loop. Floored at 100 ms to avoid a // pathologically tight loop if a bad value slips past validation. int interval = Math.Max(100, _options.CurrentValue.AdminPushIntervalMs); await Task.Delay(interval, ct).ConfigureAwait(false); - await PushOnceAsync(ct).ConfigureAwait(false); } } catch (OperationCanceledException) @@ -136,7 +153,7 @@ internal sealed class StatusBroadcaster : IAsyncDisposable } catch (Exception ex) { - _logger.LogError(ex, "StatusBroadcaster loop terminated unexpectedly"); + LogLoopTerminated(_logger, ex); } } @@ -145,4 +162,29 @@ internal sealed class StatusBroadcaster : IAsyncDisposable await StopAsync().ConfigureAwait(false); _cts.Dispose(); } + + // ── Logging ────────────────────────────────────────────────────────────── + // Stable event names in the mbproxy.admin.broadcast.* family — see + // docs/Reference/LogEvents.md. EventIds continue the admin block (70/71 in + // AdminEndpointHost). + + [LoggerMessage(EventId = 72, EventName = "mbproxy.admin.broadcast.snapshot.failed", + Level = LogLevel.Error, + Message = "Status broadcaster failed to build a status snapshot — this push cycle is skipped")] + private static partial void LogSnapshotFailed(ILogger logger, Exception ex); + + [LoggerMessage(EventId = 73, EventName = "mbproxy.admin.broadcast.fleet.failed", + Level = LogLevel.Error, + Message = "Status broadcaster failed to push the fleet snapshot to dashboard subscribers")] + private static partial void LogFleetPushFailed(ILogger logger, Exception ex); + + [LoggerMessage(EventId = 74, EventName = "mbproxy.admin.broadcast.detail.failed", + Level = LogLevel.Error, + Message = "Status broadcaster failed to push the detail snapshot for PLC {Plc}")] + private static partial void LogDetailPushFailed(ILogger logger, string plc, Exception ex); + + [LoggerMessage(EventId = 75, EventName = "mbproxy.admin.broadcast.loop.terminated", + Level = LogLevel.Error, + Message = "Status broadcaster push loop terminated unexpectedly — the live dashboard feed has stopped")] + private static partial void LogLoopTerminated(ILogger logger, Exception ex); } diff --git a/mbproxy/src/Mbproxy/Admin/wwwroot/dashboard.css b/mbproxy/src/Mbproxy/Admin/wwwroot/dashboard.css index 94784ae..915447f 100644 --- a/mbproxy/src/Mbproxy/Admin/wwwroot/dashboard.css +++ b/mbproxy/src/Mbproxy/Admin/wwwroot/dashboard.css @@ -98,14 +98,26 @@ .kpi-table th.sortable { cursor: pointer; user-select: none; } .kpi-table th.sortable:hover { color: var(--ink); } +.kpi-table th.sortable:focus-visible { + outline: 2px solid var(--accent); + outline-offset: -2px; + color: var(--ink); +} .kpi-table th.sorted-asc::after { content: ' \2191'; color: var(--accent); } .kpi-table th.sorted-desc::after { content: ' \2193'; color: var(--accent); } -.kpi-table tbody tr { cursor: pointer; transition: background 0.08s; } +.kpi-table tbody tr { transition: background 0.08s; } .kpi-table tbody tr:hover { background: #f3f6fd; } .kpi-table tbody tr:last-child td { border-bottom: none; } .kpi-table .plc-name { font-weight: 600; } +.kpi-table .plc-name a { color: inherit; text-decoration: none; } +.kpi-table .plc-name a:hover { text-decoration: underline; } +.kpi-table .plc-name a:focus-visible { + outline: 2px solid var(--accent); + outline-offset: 1px; + border-radius: 2px; +} .kpi-table .plc-host { color: var(--ink-soft); font-family: var(--mono); font-size: 0.8rem; } .empty-row { diff --git a/mbproxy/src/Mbproxy/Admin/wwwroot/dashboard.js b/mbproxy/src/Mbproxy/Admin/wwwroot/dashboard.js index 0baebc0..4863f84 100644 --- a/mbproxy/src/Mbproxy/Admin/wwwroot/dashboard.js +++ b/mbproxy/src/Mbproxy/Admin/wwwroot/dashboard.js @@ -15,6 +15,7 @@ // ── Helpers ──────────────────────────────────────────────────────────── const $ = (id) => document.getElementById(id); + const { escapeHtml, escapeAttr } = window.mbproxyUtil; function num(n) { if (n === null || n === undefined) return '—'; @@ -158,8 +159,9 @@ const excn = exceptionTotal(b); const recovHint = plc.listener.state === 'recovering' && plc.listener.lastBindError ? ` title="${escapeAttr(plc.listener.lastBindError)}"` : ''; - return ` - ${escapeHtml(plc.name)} + const plcHref = '/plc/' + encodeURIComponent(plc.name); + return ` + ${escapeHtml(plc.name)} ${escapeHtml(plc.host)}:${plc.listenPort} ${stateChip(plc.listener.state)} ${plc.clients.connected} @@ -173,13 +175,6 @@ }).join(''); } - function escapeHtml(s) { - return String(s).replace(/[&<>]/g, c => ({ '&': '&', '<': '<', '>': '>' }[c])); - } - function escapeAttr(s) { - return escapeHtml(s).replace(/"/g, '"'); - } - // ── Render orchestration ─────────────────────────────────────────────── function render() { if (!latest) return; @@ -197,6 +192,7 @@ } function formatUptime(sec) { + if (!Number.isFinite(sec) || sec < 0) return '—'; const d = Math.floor(sec / 86400); const h = Math.floor((sec % 86400) / 3600); const m = Math.floor((sec % 3600) / 60); @@ -218,21 +214,30 @@ $('f-state').addEventListener('change', e => { filter.state = e.target.value; render(); }); $('f-problems').addEventListener('change', e => { filter.problemsOnly = e.target.checked; render(); }); - document.querySelectorAll('th.sortable').forEach(th => { - th.addEventListener('click', () => { - const key = th.dataset.sort; - if (sort.key === key) { sort.dir *= -1; } - else { sort.key = key; sort.dir = 1; } - document.querySelectorAll('th.sortable').forEach(h => h.classList.remove('sorted-asc', 'sorted-desc')); - th.classList.add(sort.dir === 1 ? 'sorted-asc' : 'sorted-desc'); - render(); + // Sortable headers are keyboard-operable: each is tabindex=0 with aria-sort, + // and Enter / Space sorts just like a click. aria-sort is kept in lockstep with + // the sorted-asc / sorted-desc visual classes so a screen reader announces it. + function applySort(th) { + const key = th.dataset.sort; + if (sort.key === key) { sort.dir *= -1; } + else { sort.key = key; sort.dir = 1; } + document.querySelectorAll('th.sortable').forEach(h => { + h.classList.remove('sorted-asc', 'sorted-desc'); + h.setAttribute('aria-sort', 'none'); }); - }); + th.classList.add(sort.dir === 1 ? 'sorted-asc' : 'sorted-desc'); + th.setAttribute('aria-sort', sort.dir === 1 ? 'ascending' : 'descending'); + render(); + } - $('plc-rows').addEventListener('click', e => { - const tr = e.target.closest('tr[data-name]'); - if (!tr) return; - window.open('/plc/' + encodeURIComponent(tr.dataset.name), '_blank'); + document.querySelectorAll('th.sortable').forEach(th => { + th.addEventListener('click', () => applySort(th)); + th.addEventListener('keydown', e => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + applySort(th); + } + }); }); } @@ -273,7 +278,9 @@ // ── Boot ─────────────────────────────────────────────────────────────── document.addEventListener('DOMContentLoaded', () => { wireControls(); - document.querySelector('th[data-sort="name"]').classList.add('sorted-asc'); + const nameTh = document.querySelector('th[data-sort="name"]'); + nameTh.classList.add('sorted-asc'); + nameTh.setAttribute('aria-sort', 'ascending'); connect(); }); })(); diff --git a/mbproxy/src/Mbproxy/Admin/wwwroot/detail.js b/mbproxy/src/Mbproxy/Admin/wwwroot/detail.js index 539f5ed..69a5507 100644 --- a/mbproxy/src/Mbproxy/Admin/wwwroot/detail.js +++ b/mbproxy/src/Mbproxy/Admin/wwwroot/detail.js @@ -7,9 +7,20 @@ (function () { // ── PLC name from the URL path: /plc/{name} ──────────────────────────── - const plcName = decodeURIComponent(location.pathname.replace(/^\/plc\//, '')); + // decodeURIComponent throws URIError on a malformed %-escape; fall back to the + // raw path segment and flag the failure so the boot path shows a notice instead + // of letting the whole script abort on the very first statement. + const rawSegment = location.pathname.replace(/^\/plc\//, ''); + let plcName, plcNameError = false; + try { + plcName = decodeURIComponent(rawSegment); + } catch { + plcName = rawSegment; + plcNameError = true; + } const $ = (id) => document.getElementById(id); + const { escapeHtml } = window.mbproxyUtil; document.title = `mbproxy — ${plcName}`; $('crumb-name').textContent = plcName; @@ -20,9 +31,6 @@ if (n === null || n === undefined) return '—'; return n.toLocaleString('en-US'); } - function escapeHtml(s) { - return String(s).replace(/[&<>]/g, c => ({ '&': '&', '<': '<', '>': '>' }[c])); - } function hex4(n) { return '0x' + (n & 0xffff).toString(16).toUpperCase().padStart(4, '0'); } // First debug-row cell: the tag's friendly name (when configured) over its PDU @@ -37,7 +45,7 @@ } function formatAge(sec) { - if (sec === null || sec === undefined) return '—'; + if (!Number.isFinite(sec) || sec < 0) return '—'; if (sec < 1) return 'now'; if (sec < 60) return sec.toFixed(1) + 's'; const m = Math.floor(sec / 60); @@ -299,5 +307,12 @@ } } - document.addEventListener('DOMContentLoaded', connect); + document.addEventListener('DOMContentLoaded', () => { + if (plcNameError) { + showNotice('The PLC name in this URL could not be decoded — the address is ' + + 'malformed. Return to the fleet page and open the PLC from the table.'); + return; + } + connect(); + }); })(); diff --git a/mbproxy/src/Mbproxy/Admin/wwwroot/index.html b/mbproxy/src/Mbproxy/Admin/wwwroot/index.html index 6672468..bc8c579 100644 --- a/mbproxy/src/Mbproxy/Admin/wwwroot/index.html +++ b/mbproxy/src/Mbproxy/Admin/wwwroot/index.html @@ -14,7 +14,7 @@ fleet — - + connecting @@ -52,8 +52,10 @@
- @@ -69,16 +71,16 @@ - - - - - - - - - - + + + + + + + + + + @@ -91,6 +93,7 @@ + diff --git a/mbproxy/src/Mbproxy/Admin/wwwroot/plc.html b/mbproxy/src/Mbproxy/Admin/wwwroot/plc.html index 46be61f..a0428dd 100644 --- a/mbproxy/src/Mbproxy/Admin/wwwroot/plc.html +++ b/mbproxy/src/Mbproxy/Admin/wwwroot/plc.html @@ -15,7 +15,7 @@ ›— - + connecting @@ -67,6 +67,7 @@ + diff --git a/mbproxy/src/Mbproxy/Admin/wwwroot/util.js b/mbproxy/src/Mbproxy/Admin/wwwroot/util.js new file mode 100644 index 0000000..ef484ab --- /dev/null +++ b/mbproxy/src/Mbproxy/Admin/wwwroot/util.js @@ -0,0 +1,20 @@ +/* ============================================================================ + Shared helpers for the admin dashboard pages. Loaded before dashboard.js / + detail.js; each page's IIFE pulls these off window.mbproxyUtil so the HTML- + escaping logic has exactly one definition. + ========================================================================= */ +'use strict'; + +(function () { + /** Escapes the three HTML-significant characters for safe text-node insertion. */ + function escapeHtml(s) { + return String(s).replace(/[&<>]/g, c => ({ '&': '&', '<': '<', '>': '>' }[c])); + } + + /** Escapes a value for use inside a double-quoted HTML attribute. */ + function escapeAttr(s) { + return escapeHtml(s).replace(/"/g, '"'); + } + + window.mbproxyUtil = { escapeHtml, escapeAttr }; +})(); diff --git a/mbproxy/src/Mbproxy/Configuration/ReloadValidator.cs b/mbproxy/src/Mbproxy/Configuration/ReloadValidator.cs index 93b1861..f986e49 100644 --- a/mbproxy/src/Mbproxy/Configuration/ReloadValidator.cs +++ b/mbproxy/src/Mbproxy/Configuration/ReloadValidator.cs @@ -72,8 +72,9 @@ internal static class ReloadValidator errs.Add($"AdminPort {adminPort} collides with ListenPort of PLC '{clashPlc}'."); } - if (next.AdminPushIntervalMs <= 0) - errs.Add($"AdminPushIntervalMs must be > 0; got {next.AdminPushIntervalMs}."); + if (next.AdminPushIntervalMs <= 0 || next.AdminPushIntervalMs > 60_000) + errs.Add( + $"AdminPushIntervalMs must be between 1 and 60000 ms; got {next.AdminPushIntervalMs}."); // ── 4. Per-PLC tag-map build ────────────────────────────────────────── // BcdTagMapBuilder.Build is the single source of truth for tag-list diff --git a/mbproxy/src/Mbproxy/Options/MbproxyOptions.cs b/mbproxy/src/Mbproxy/Options/MbproxyOptions.cs index a89aa6a..1bb40f6 100644 --- a/mbproxy/src/Mbproxy/Options/MbproxyOptions.cs +++ b/mbproxy/src/Mbproxy/Options/MbproxyOptions.cs @@ -11,7 +11,8 @@ public sealed class MbproxyOptions /// /// Server-push cadence (milliseconds) for the admin dashboard's SignalR feed. /// Every interval the admin endpoint builds a status snapshot and pushes it to - /// connected dashboard / detail-page clients. Must be > 0. Defaults to 1000. + /// connected dashboard / detail-page clients. Must be in the range 1–60000 ms + /// (a value past a minute makes the "live" feed non-live). Defaults to 1000. /// public int AdminPushIntervalMs { get; init; } = 1000; @@ -114,8 +115,12 @@ public sealed class MbproxyOptionsValidator : IValidateOptions errors.Add( $"Connection.GracefulShutdownTimeoutMs must be > 0; got {options.Connection.GracefulShutdownTimeoutMs}."); - if (options.AdminPushIntervalMs <= 0) - errors.Add($"AdminPushIntervalMs must be > 0; got {options.AdminPushIntervalMs}."); + // AdminPushIntervalMs has a soft upper bound: a value past a minute makes the + // dashboard's "live" feed effectively non-live, which is almost always a typo + // (e.g. a seconds value pasted as milliseconds) rather than an intent. + if (options.AdminPushIntervalMs <= 0 || options.AdminPushIntervalMs > 60_000) + errors.Add( + $"AdminPushIntervalMs must be between 1 and 60000 ms; got {options.AdminPushIntervalMs}."); // Keepalive section ranges. Cross-field rules (heartbeat interval vs request // timeout) are enforced in ReloadValidator. diff --git a/mbproxy/tests/Mbproxy.Tests/Admin/AdminEndpointTests.cs b/mbproxy/tests/Mbproxy.Tests/Admin/AdminEndpointTests.cs index 5d5cfc3..8778117 100644 --- a/mbproxy/tests/Mbproxy.Tests/Admin/AdminEndpointTests.cs +++ b/mbproxy/tests/Mbproxy.Tests/Admin/AdminEndpointTests.cs @@ -243,7 +243,22 @@ public sealed class AdminEndpointTests response.Headers.CacheControl?.ToString().ShouldContain("immutable"); var bytes = await response.Content.ReadAsByteArrayAsync(TestContext.Current.CancellationToken); - bytes.Length.ShouldBeGreaterThan(0); + + // The served bytes must be the actual embedded asset — not some other resource + // of the same length. Compare against the manifest resource directly. + byte[] expected = ReadEmbeddedAsset(file); + bytes.ShouldBe(expected, $"GET /assets/{file} must return the embedded asset verbatim"); + } + + /// Reads a wwwroot asset straight from the assembly's manifest resources. + private static byte[] ReadEmbeddedAsset(string fileName) + { + using var stream = typeof(Mbproxy.Admin.StatusHub).Assembly + .GetManifestResourceStream("Mbproxy.Admin.wwwroot." + fileName) + ?? throw new InvalidOperationException($"Embedded asset not found: {fileName}"); + using var ms = new MemoryStream(); + stream.CopyTo(ms); + return ms.ToArray(); } [Fact(Timeout = 5_000)] diff --git a/mbproxy/tests/Mbproxy.Tests/Admin/EmbeddedAssetsTests.cs b/mbproxy/tests/Mbproxy.Tests/Admin/EmbeddedAssetsTests.cs new file mode 100644 index 0000000..9630bc1 --- /dev/null +++ b/mbproxy/tests/Mbproxy.Tests/Admin/EmbeddedAssetsTests.cs @@ -0,0 +1,63 @@ +using System.Reflection; +using Mbproxy.Admin; +using Shouldly; +using Xunit; + +namespace Mbproxy.Tests.Admin; + +/// +/// Guards the Admin\wwwroot\*.* embedded-resource glob in Mbproxy.csproj. +/// A broken or narrowed glob would silently drop a UI asset from the single-file binary; +/// the admin endpoint would then 404 it at runtime with no compile-time failure. This +/// test fails the build instead by comparing the on-disk source folder against the +/// assembly's manifest resources. +/// +[Trait("Category", "Unit")] +public sealed class EmbeddedAssetsTests +{ + private const string ResourcePrefix = "Mbproxy.Admin.wwwroot."; + + [Fact] + public void EveryWwwrootFile_IsEmbeddedAsAManifestResource() + { + var sourceDir = LocateWwwrootSource(); + var sourceFiles = Directory.GetFiles(sourceDir) + .Select(Path.GetFileName) + .Where(n => n is not null) + .Select(n => n!) + .ToArray(); + + sourceFiles.ShouldNotBeEmpty("the source wwwroot folder should contain UI assets"); + + var embedded = typeof(StatusHub).Assembly + .GetManifestResourceNames() + .Where(n => n.StartsWith(ResourcePrefix, StringComparison.Ordinal)) + .ToHashSet(StringComparer.Ordinal); + + foreach (var file in sourceFiles) + { + embedded.ShouldContain(ResourcePrefix + file, + $"wwwroot asset '{file}' is not embedded — check the EmbeddedResource glob in Mbproxy.csproj"); + } + } + + /// + /// Walks up from the test assembly directory to the repo and returns + /// src/Mbproxy/Admin/wwwroot — same upward-search pattern the simulator + /// fixture uses to find tests/sim. + /// + private static string LocateWwwrootSource() + { + var dir = new DirectoryInfo( + Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location) ?? "."); + while (dir is not null) + { + var candidate = Path.Combine(dir.FullName, "src", "Mbproxy", "Admin", "wwwroot"); + if (Directory.Exists(candidate)) + return candidate; + dir = dir.Parent; + } + throw new DirectoryNotFoundException( + "Could not locate src/Mbproxy/Admin/wwwroot above the test assembly."); + } +} diff --git a/mbproxy/tests/Mbproxy.Tests/Admin/HubStatusE2ETests.cs b/mbproxy/tests/Mbproxy.Tests/Admin/HubStatusE2ETests.cs new file mode 100644 index 0000000..efa7c9a --- /dev/null +++ b/mbproxy/tests/Mbproxy.Tests/Admin/HubStatusE2ETests.cs @@ -0,0 +1,160 @@ +using System.Net; +using System.Net.Http; +using System.Net.Sockets; +using System.Text.Json; +using Mbproxy.Options; +using Mbproxy.Proxy; +using Microsoft.AspNetCore.SignalR.Client; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Serilog; +using Shouldly; +using Xunit; + +namespace Mbproxy.Tests.Admin; + +/// +/// End-to-end test of the SignalR live feed: a real against +/// the live Kestrel admin host. Exercises the whole push path that no other test covers — +/// group joins, the MapHub wiring, the +/// , and the broadcaster loop — +/// confirming that SubscribeFleet yields a "fleet" message and +/// SubscribePlc yields a "plc" message. +/// +[Trait("Category", "E2E")] +public sealed class HubStatusE2ETests +{ + [Fact(Timeout = 15_000)] + public async Task SubscribeFleet_ReceivesFleetSnapshot() + { + int adminPort = PickFreePort(); + int proxyPort = PickFreePort(); + + await using var host = BuildHost(adminPort, proxyPort); + using var startCts = new CancellationTokenSource(TimeSpan.FromSeconds(10)); + await host.Host.StartAsync(startCts.Token); + await WaitForAdminAsync(adminPort); + + await using var connection = new HubConnectionBuilder() + .WithUrl($"http://127.0.0.1:{adminPort}/hub/status") + .Build(); + + var fleet = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + connection.On("fleet", payload => fleet.TrySetResult(payload)); + + await connection.StartAsync(TestContext.Current.CancellationToken); + await connection.InvokeAsync("SubscribeFleet", TestContext.Current.CancellationToken); + + var snapshot = await fleet.Task.WaitAsync( + TimeSpan.FromSeconds(8), TestContext.Current.CancellationToken); + + // The fleet payload is a StatusResponse — assert a couple of its known fields. + snapshot.TryGetProperty("service", out _).ShouldBeTrue("fleet payload must carry 'service'"); + snapshot.TryGetProperty("plcs", out var plcs).ShouldBeTrue("fleet payload must carry 'plcs'"); + plcs.ValueKind.ShouldBe(JsonValueKind.Array); + } + + [Fact(Timeout = 15_000)] + public async Task SubscribePlc_ReceivesDetailSnapshot() + { + int adminPort = PickFreePort(); + int proxyPort = PickFreePort(); + + await using var host = BuildHost(adminPort, proxyPort); + using var startCts = new CancellationTokenSource(TimeSpan.FromSeconds(10)); + await host.Host.StartAsync(startCts.Token); + await WaitForAdminAsync(adminPort); + + await using var connection = new HubConnectionBuilder() + .WithUrl($"http://127.0.0.1:{adminPort}/hub/status") + .Build(); + + var detail = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + connection.On("plc", payload => detail.TrySetResult(payload)); + + await connection.StartAsync(TestContext.Current.CancellationToken); + // tabId is a stable per-page-load identifier the real client generates. + await connection.InvokeAsync("SubscribePlc", "TestPLC", "tab-e2e", + TestContext.Current.CancellationToken); + + var snapshot = await detail.Task.WaitAsync( + TimeSpan.FromSeconds(8), TestContext.Current.CancellationToken); + + // The detail payload is a PlcDetailResponse { plc, debug }. + snapshot.TryGetProperty("debug", out var debug).ShouldBeTrue("detail payload must carry 'debug'"); + debug.TryGetProperty("captureArmed", out _).ShouldBeTrue("debug must carry 'captureArmed'"); + debug.TryGetProperty("tags", out var tags).ShouldBeTrue("debug must carry 'tags'"); + tags.ValueKind.ShouldBe(JsonValueKind.Array); + } + + // ── Helpers ─────────────────────────────────────────────────────────────── + + private static HostHandle BuildHost(int adminPort, int proxyPort) + { + var config = new Dictionary + { + ["Mbproxy:AdminPort"] = adminPort.ToString(), + // Fast push cadence so the subscribed client sees a message promptly. + ["Mbproxy:AdminPushIntervalMs"] = "100", + ["Mbproxy:Plcs:0:Name"] = "TestPLC", + ["Mbproxy:Plcs:0:ListenPort"] = proxyPort.ToString(), + ["Mbproxy:Plcs:0:Host"] = "127.0.0.1", + ["Mbproxy:Plcs:0:Port"] = "502", + ["Mbproxy:Connection:BackendConnectTimeoutMs"] = "500", + ["Mbproxy:Connection:BackendRequestTimeoutMs"] = "500", + }; + + var builder = Host.CreateApplicationBuilder(); + builder.Configuration.AddInMemoryCollection(config); + builder.Services.AddSerilog( + new LoggerConfiguration().MinimumLevel.Fatal().CreateLogger(), dispose: false); + builder.AddMbproxyOptions(); + builder.Services.AddSingleton(); + builder.Services.AddSingleton(); + builder.Services.AddHostedService(sp => sp.GetRequiredService()); + builder.AddMbproxyAdmin(); + + return new HostHandle(builder.Build()); + } + + private static async Task WaitForAdminAsync(int adminPort) + { + using var http = new HttpClient(); + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(10)); + while (!cts.IsCancellationRequested) + { + try + { + var r = await http.GetAsync($"http://127.0.0.1:{adminPort}/status.json", cts.Token); + if (r.StatusCode == HttpStatusCode.OK) return; + } + catch { } + await Task.Delay(100, cts.Token).ConfigureAwait(false); + } + throw new TimeoutException($"Admin endpoint on port {adminPort} did not start in time."); + } + + private static int PickFreePort() + { + var l = new TcpListener(IPAddress.Loopback, 0); + l.Start(); + int port = ((IPEndPoint)l.LocalEndpoint).Port; + l.Stop(); + return port; + } + + private sealed class HostHandle(IHost host) : IAsyncDisposable + { + public IHost Host { get; } = host; + + public async ValueTask DisposeAsync() + { + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(5)); + try { await Host.StopAsync(cts.Token); } catch { } + Host.Dispose(); + } + } +} diff --git a/mbproxy/tests/Mbproxy.Tests/Admin/SignalRFakes.cs b/mbproxy/tests/Mbproxy.Tests/Admin/SignalRFakes.cs index 1b332bc..25f8d4b 100644 --- a/mbproxy/tests/Mbproxy.Tests/Admin/SignalRFakes.cs +++ b/mbproxy/tests/Mbproxy.Tests/Admin/SignalRFakes.cs @@ -64,3 +64,18 @@ internal sealed class FakeStatusPushSink : IStatusPushSink return Task.CompletedTask; } } + +/// +/// An whose every push fails with an exception from the +/// supplied factory — used to prove swallows +/// a transport fault (and that its when (ex is not OperationCanceledException) filter +/// still lets a cancellation propagate). +/// +internal sealed class ThrowingStatusPushSink(Func exceptionFactory) : IStatusPushSink +{ + public Task PushFleetAsync(StatusResponse snapshot, CancellationToken ct) + => throw exceptionFactory(); + + public Task PushPlcAsync(string plcName, PlcDetailResponse detail, CancellationToken ct) + => throw exceptionFactory(); +} diff --git a/mbproxy/tests/Mbproxy.Tests/Admin/StatusBroadcasterTests.cs b/mbproxy/tests/Mbproxy.Tests/Admin/StatusBroadcasterTests.cs index d6f70fc..a62bbee 100644 --- a/mbproxy/tests/Mbproxy.Tests/Admin/StatusBroadcasterTests.cs +++ b/mbproxy/tests/Mbproxy.Tests/Admin/StatusBroadcasterTests.cs @@ -39,7 +39,7 @@ public sealed class StatusBroadcasterTests } } - private static async Task BuildAsync() + private static async Task BuildAsync(IStatusPushSink? sinkOverride = null) { var hostBuilder = Host.CreateApplicationBuilder(); hostBuilder.Configuration.AddInMemoryCollection(new Dictionary @@ -68,7 +68,7 @@ public sealed class StatusBroadcasterTests var sink = new FakeStatusPushSink(); var broadcaster = new StatusBroadcaster( - sink, builder, tracker, registry, options, NullLogger.Instance); + sinkOverride ?? sink, builder, tracker, registry, options, NullLogger.Instance); return new Harness(host, broadcaster, sink, builder, registry, tracker); } @@ -129,6 +129,31 @@ public sealed class StatusBroadcasterTests capture.IsArmed.ShouldBeFalse("the broadcaster disarms a capture once its last viewer leaves"); } + [Fact] + public async Task PushOnce_SinkThrowsNonCancellation_FailureIsSwallowed() + { + // A SignalR transport fault on a push must not escape PushOnceAsync — the loop + // has to survive it and retry on the next cycle. + var throwing = new ThrowingStatusPushSink(() => new InvalidOperationException("boom")); + await using var h = await BuildAsync(throwing); + + await Should.NotThrowAsync( + () => h.Broadcaster.PushOnceAsync(TestContext.Current.CancellationToken)); + } + + [Fact] + public async Task PushOnce_SinkThrowsOperationCanceled_Propagates() + { + // The catch filters are `when (ex is not OperationCanceledException)` — a genuine + // cancellation must propagate so the loop unwinds at shutdown instead of being + // swallowed and retried. + var throwing = new ThrowingStatusPushSink(() => new OperationCanceledException()); + await using var h = await BuildAsync(throwing); + + await Should.ThrowAsync( + () => h.Broadcaster.PushOnceAsync(TestContext.Current.CancellationToken)); + } + [Fact] public async Task StopAsync_DisarmsEveryCapture() { diff --git a/mbproxy/tests/Mbproxy.Tests/Configuration/ConfigReconcilerTests.cs b/mbproxy/tests/Mbproxy.Tests/Configuration/ConfigReconcilerTests.cs index 093aaf4..18f0464 100644 --- a/mbproxy/tests/Mbproxy.Tests/Configuration/ConfigReconcilerTests.cs +++ b/mbproxy/tests/Mbproxy.Tests/Configuration/ConfigReconcilerTests.cs @@ -68,13 +68,14 @@ public sealed class ConfigReconcilerTests : IAsyncDisposable private ConfigReconciler BuildReconciler( IOptionsMonitor monitor, - ServiceCounters? counters = null) + ServiceCounters? counters = null, + Mbproxy.Proxy.TagCaptureRegistry? captureRegistry = null) { return new ConfigReconciler( monitor, NullLoggerFactory.Instance, counters ?? new ServiceCounters(), - new Mbproxy.Proxy.TagCaptureRegistry()); + captureRegistry ?? new Mbproxy.Proxy.TagCaptureRegistry()); } // The reconciler and supervisors tracked for cleanup. @@ -347,6 +348,53 @@ public sealed class ConfigReconcilerTests : IAsyncDisposable foreach (var s in supervisors.Values) _supervisors.Add(s); } + + // ── Test 6: Reconciler ↔ TagCaptureRegistry wiring ──────────────────────────────────── + + /// + /// The reconciler owns the tag-capture lifecycle for hot-reload: a PLC added by a + /// reload must get a capture entry (GetOrCreate), and a PLC removed by a + /// reload must have its capture entry dropped (Remove). Holds a real + /// and asserts TryGet tracks + /// the roster across an add reload and a remove reload. + /// + [Fact] + public async Task Apply_AddThenRemovePlc_TagCaptureRegistryTracksRoster() + { + int portA = PickFreePort(); + int portB = PickFreePort(); + + var plcA = MakePlc("A", portA); + var plcB = MakePlc("B", portB); + var initial = MakeOptions([plcA]); + var withB = MakeOptions([plcA, plcB]); + + var supA = BuildSupervisor(plcA); + _supervisors.Add(supA); + await supA.StartAsync(CancellationToken.None); + + var supervisors = new ConcurrentDictionary(StringComparer.Ordinal) + { + ["A"] = supA, + }; + + var registry = new Mbproxy.Proxy.TagCaptureRegistry(); + var monitor = new FakeOptionsMonitor(initial); + var reconciler = BuildReconciler(monitor, captureRegistry: registry); + _reconcilers.Add(reconciler); + reconciler.Attach(supervisors, initial); + + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(10)); + + // Reload that adds PLC-B → the registry must gain a capture for B. + Assert.True(await reconciler.ApplyAsync(withB, cts.Token)); + Assert.True(registry.TryGet("B", out _), "adding PLC-B must create its tag-value capture"); + _supervisors.Add(supervisors["B"]); + + // Reload that removes PLC-B → the registry must drop B's capture. + Assert.True(await reconciler.ApplyAsync(initial, cts.Token)); + Assert.False(registry.TryGet("B", out _), "removing PLC-B must drop its tag-value capture"); + } } /// diff --git a/mbproxy/tests/Mbproxy.Tests/Configuration/ReloadValidatorTests.cs b/mbproxy/tests/Mbproxy.Tests/Configuration/ReloadValidatorTests.cs index 79bd73a..3af2be1 100644 --- a/mbproxy/tests/Mbproxy.Tests/Configuration/ReloadValidatorTests.cs +++ b/mbproxy/tests/Mbproxy.Tests/Configuration/ReloadValidatorTests.cs @@ -366,4 +366,35 @@ public sealed class ReloadValidatorTests Assert.False(valid); Assert.Contains(errors, e => e.Contains("AdminPushIntervalMs")); } + + [Fact] + public void Validate_AdminPushIntervalMs_AboveUpperBound_Fails() + { + // The soft upper bound (60 s) catches a seconds-as-milliseconds typo that + // would make the "live" dashboard feed effectively non-live. + var opts = new MbproxyOptions + { + Plcs = [MakePlc("PLC-A", 5020)], + AdminPushIntervalMs = 60_001, + }; + + bool valid = ReloadValidator.Validate(opts, out var errors); + + Assert.False(valid); + Assert.Contains(errors, e => e.Contains("AdminPushIntervalMs")); + } + + [Fact] + public void Validate_AdminPushIntervalMs_AtUpperBound_Passes() + { + var opts = new MbproxyOptions + { + Plcs = [MakePlc("PLC-A", 5020)], + AdminPushIntervalMs = 60_000, + }; + + bool valid = ReloadValidator.Validate(opts, out var errors); + + Assert.True(valid, string.Join("; ", errors)); + } } diff --git a/mbproxy/tests/Mbproxy.Tests/Mbproxy.Tests.csproj b/mbproxy/tests/Mbproxy.Tests/Mbproxy.Tests.csproj index 1268992..5ad1c04 100644 --- a/mbproxy/tests/Mbproxy.Tests/Mbproxy.Tests.csproj +++ b/mbproxy/tests/Mbproxy.Tests/Mbproxy.Tests.csproj @@ -23,6 +23,9 @@ + + diff --git a/mbproxy/tests/sim/mbproxy.smoke.config.json b/mbproxy/tests/sim/mbproxy.smoke.config.json index 2577286..01988d9 100644 --- a/mbproxy/tests/sim/mbproxy.smoke.config.json +++ b/mbproxy/tests/sim/mbproxy.smoke.config.json @@ -1,5 +1,6 @@ -// mbproxy smoke-test configuration — used by the Phase 4/5 web-UI browser smoke -// tests (see plans/2026-05-15-webui-dashboard.md). NOT a deployment config. +// mbproxy smoke-test configuration for the web-UI browser smoke tests. +// NOT a deployment config. The Resilience and Cache sections are intentionally +// omitted — the smoke run relies on their built-in defaults. // // Topology: // * line-a / line-b → the dl205 simulator on 127.0.0.1:5020 (run-dl205-sim.ps1).
PLCBackendStateClientsPDU/sRTT msExcnsCoalesceCacheKeepalivePLCBackendStateClientsPDU/sRTT msExcnsCoalesceCacheKeepalive