diff --git a/mbproxy/docs/Operations/StatusPage.md b/mbproxy/docs/Operations/StatusPage.md index 8adcf2a..9e6a02d 100644 --- a/mbproxy/docs/Operations/StatusPage.md +++ b/mbproxy/docs/Operations/StatusPage.md @@ -312,7 +312,7 @@ The UI is a Bootstrap 5 single-page app served from embedded assets under `src/M ## Debug View Data -The detail page's debug view is fed by an **on-demand per-tag value capture** (`Proxy/TagValueCapture.cs`, one per PLC, held in `Proxy/TagCaptureRegistry.cs`). The `BcdPduPipeline` records the last raw/decoded value for each configured BCD tag — but only while the capture is *armed*. `StatusHub` arms a PLC's capture when the first detail page subscribes and disarms it (clearing all slots) when the last viewer leaves, so the hot path carries zero cost when nobody is watching. The per-PLC payload is `PlcDetailResponse` (`src/Mbproxy/Admin/DebugDto.cs`): +The detail page's debug view is fed by an **on-demand per-tag value capture** (`Proxy/TagValueCapture.cs`, one per PLC, held in `Proxy/TagCaptureRegistry.cs`). The `BcdPduPipeline` records the last raw/decoded value for each configured BCD tag — but only while the capture is *armed*. `StatusBroadcaster` reconciles arm state every push cycle from `PlcSubscriptionTracker`: a PLC's capture is armed exactly while at least one detail-page browser tab is open, and disarmed (clearing all slots) otherwise — so the hot path carries zero cost when nobody is watching. The tracker keys on a stable per-page-load tab id, not the SignalR `ConnectionId`, so a transport reconnect cannot leak an armed capture. The per-PLC payload is `PlcDetailResponse` (`src/Mbproxy/Admin/DebugDto.cs`): > When the response cache is enabled, an FC03/FC04 **cache hit** bypasses the pipeline. To keep the debug view live for cached tags, each cache entry carries the tag observations captured when it was stored (only when a viewer was armed at that time); a hit replays them into the capture, re-stamped to the hit time. The debug view therefore reflects the value the client actually receives — cache-served reads included — not only backend round-trips. diff --git a/mbproxy/src/Mbproxy/Admin/AdminEndpointHost.cs b/mbproxy/src/Mbproxy/Admin/AdminEndpointHost.cs index e30535f..60deb71 100644 --- a/mbproxy/src/Mbproxy/Admin/AdminEndpointHost.cs +++ b/mbproxy/src/Mbproxy/Admin/AdminEndpointHost.cs @@ -166,6 +166,10 @@ internal sealed partial class AdminEndpointHost : IAsyncDisposable /// private async Task StartAppAsync(int port, CancellationToken ct) { + // Declared outside the try so the catch can dispose a built-but-not-fully-started + // app on a bind failure (M6 — otherwise a built WebApplication or a started + // Kestrel listener leaks on any throw after Build()). + WebApplication? app = null; try { // Use CreateSlimBuilder with explicit args (empty) to avoid inheriting @@ -187,17 +191,18 @@ internal sealed partial class AdminEndpointHost : IAsyncDisposable }); // SignalR hub for the live dashboard. The inner WebApplication has its own - // DI container, so the singletons StatusHub depends on are re-registered here. - // camelCase payloads keep the wire shape identical to GET /status.json. + // DI container, so the singleton StatusHub depends on is re-registered here. + // The payload serialises via reflection-based System.Text.Json with a + // camelCase policy — the same wire shape as GET /status.json. The project + // does not trim/AOT, so a reflection JSON path is acceptable here. builder.Services .AddSignalR() .AddJsonProtocol(o => o.PayloadSerializerOptions.PropertyNamingPolicy = System.Text.Json.JsonNamingPolicy.CamelCase); - builder.Services.AddSingleton(_captureRegistry); builder.Services.AddSingleton(_subscriptionTracker); - var app = builder.Build(); + app = builder.Build(); // ── Routes ─────────────────────────────────────────────────────── // GET / — fleet dashboard SPA shell @@ -252,9 +257,26 @@ internal sealed partial class AdminEndpointHost : IAsyncDisposable } catch (Exception ex) when (ex is not OperationCanceledException) { - // Bind failed — log and continue. Proxy listeners are unaffected. + // Bind (or post-bind setup) failed — log and continue. Proxy listeners are + // unaffected. Tear down anything that started before the failure so neither + // the push loop nor a bound Kestrel listener leaks (M6). LogAdminBindFailed(_logger, port, ex.Message); + + if (_broadcaster is { } broadcaster) + { + _broadcaster = null; + try { await broadcaster.DisposeAsync().ConfigureAwait(false); } + catch { /* best-effort */ } + } + _app = null; + if (app is not null) + { + try { await app.StopAsync().ConfigureAwait(false); } + catch { /* best-effort — may never have started */ } + try { await app.DisposeAsync().ConfigureAwait(false); } + catch { /* best-effort */ } + } } } diff --git a/mbproxy/src/Mbproxy/Admin/DebugDto.cs b/mbproxy/src/Mbproxy/Admin/DebugDto.cs index f94795f..f8a5d8a 100644 --- a/mbproxy/src/Mbproxy/Admin/DebugDto.cs +++ b/mbproxy/src/Mbproxy/Admin/DebugDto.cs @@ -1,10 +1,10 @@ -using System.Text.Json.Serialization; - namespace Mbproxy.Admin; // ── Wire DTOs for the connection-detail debug view ─────────────────────────── -// Pushed over SignalR to subscribers of a single PLC's detail page. camelCase via -// JsonKnownNamingPolicy.CamelCase on the source-gen context below. +// Pushed over SignalR to subscribers of a single PLC's detail page. The SignalR hub +// serialises these via reflection-based System.Text.Json with a camelCase property +// policy (see AdminEndpointHost's AddJsonProtocol) — the same wire shape as +// GET /status.json. The project does not trim/AOT, so the reflection path is fine. /// /// Per-PLC payload pushed to detail-page subscribers: the standard per-PLC status @@ -52,12 +52,3 @@ public sealed record TagValueDto( string? UpdatedAtUtc, /// Seconds since the observation; null when no traffic yet. double? AgeSeconds); - -// ── Source-generation context ───────────────────────────────────────────────── - -[JsonSerializable(typeof(PlcDetailResponse))] -[JsonSerializable(typeof(PlcDebugSnapshot))] -[JsonSourceGenerationOptions( - WriteIndented = false, - PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase)] -internal partial class DebugJsonContext : JsonSerializerContext; diff --git a/mbproxy/src/Mbproxy/Admin/PlcSubscriptionTracker.cs b/mbproxy/src/Mbproxy/Admin/PlcSubscriptionTracker.cs index 6413012..a758e17 100644 --- a/mbproxy/src/Mbproxy/Admin/PlcSubscriptionTracker.cs +++ b/mbproxy/src/Mbproxy/Admin/PlcSubscriptionTracker.cs @@ -1,83 +1,109 @@ namespace Mbproxy.Admin; /// -/// Tracks which SignalR connections are subscribed to which PLC detail pages, so the -/// admin layer knows (a) when to arm / disarm a PLC's tag-value capture — capture is -/// armed only while at least one detail page is open — and (b) which PLC groups the -/// needs to push to. +/// Tracks which browser tabs currently have a PLC connection-detail page open, so the +/// admin layer knows which PLC tag-value captures should be armed and which PLC groups +/// the needs to push to. /// -/// Registered as a DI singleton; instances (transient, -/// one per hub call) share this single tracker. All methods are thread-safe under a -/// single lock — subscription churn is low-frequency (one event per detail-page -/// open/close), so lock contention is a non-issue. +/// Why tabs, not SignalR connections. A SignalR connection is assigned a +/// fresh ConnectionId on every transport reconnect (a WebSocket drop, a +/// long-polling cycle, a network blip). Counting connections therefore leaks a +/// subscriber on every reconnect — the old connection's OnDisconnectedAsync is +/// not ordered against the new connection's re-subscribe, so the count never returns to +/// 0 and the capture stays armed forever with no viewer. Instead each detail page sends +/// a stable per-page-load tabId: a tab "views" a PLC for as long as it has at +/// least one live connection. A reconnect is just the same tab acquiring a new +/// connection, so it cannot leak; a tab is released only when its last +/// connection is gone (the page closed, or SignalR's keepalive timeout elapsed for an +/// abruptly-killed tab). +/// +/// Registered as a DI singleton; the transient instances +/// share this one tracker. All methods are thread-safe under a single lock — +/// subscription churn is low-frequency (one event per detail-page open / close / +/// reconnect), so lock contention is a non-issue. The tracker never arms or disarms a +/// capture itself — reconciles arm state each push +/// cycle from , which keeps arming single-threaded. /// internal sealed class PlcSubscriptionTracker { + /// Live state for one browser tab: its connections and the PLCs it views. + private sealed class TabState + { + public readonly HashSet Connections = new(StringComparer.Ordinal); + public readonly HashSet Plcs = new(StringComparer.Ordinal); + } + private readonly object _gate = new(); - // PLC name → number of connections currently subscribed to its detail page. - private readonly Dictionary _plcCounts = new(StringComparer.Ordinal); + // tabId → tab state (live connections + the PLC detail pages it has open). + private readonly Dictionary _tabs = new(StringComparer.Ordinal); - // Connection id → the set of PLC names that connection is subscribed to. - private readonly Dictionary> _byConnection = new(StringComparer.Ordinal); + // connectionId → owning tabId, so a disconnect can find (and decrement) its tab. + private readonly Dictionary _connToTab = new(StringComparer.Ordinal); + + // PLC name → number of distinct tabs currently viewing its detail page. + private readonly Dictionary _plcViewerTabs = new(StringComparer.Ordinal); /// - /// Records that subscribed to . - /// Returns true when this is the PLC's first subscriber (count 0 → 1), the - /// signal to arm its capture. Returns false for a redundant re-subscribe. + /// Records that connection , belonging to browser tab + /// , has the detail page for open. + /// Idempotent: a reconnect (same tab, new connection) or a repeated call for an + /// already-subscribed tag does not double-count the tab. /// - public bool Add(string connectionId, string plcName) + public void SubscribePlc(string connectionId, string tabId, string plcName) { lock (_gate) { - if (!_byConnection.TryGetValue(connectionId, out var set)) - _byConnection[connectionId] = set = new HashSet(StringComparer.Ordinal); + if (!_tabs.TryGetValue(tabId, out var tab)) + _tabs[tabId] = tab = new TabState(); - if (!set.Add(plcName)) - return false; // this connection was already subscribed to this PLC + tab.Connections.Add(connectionId); + _connToTab[connectionId] = tabId; - int count = _plcCounts.GetValueOrDefault(plcName); - _plcCounts[plcName] = count + 1; - return count == 0; + if (tab.Plcs.Add(plcName)) + _plcViewerTabs[plcName] = _plcViewerTabs.GetValueOrDefault(plcName) + 1; } } /// - /// Drops every subscription held by (called from - /// ). Returns the PLC names whose - /// subscriber count fell to 0 — the signal to disarm their captures. + /// Drops connection . If it was the last live + /// connection of its tab, the tab is released and its PLC subscriptions decremented. + /// A still-live sibling connection (reconnect overlap) keeps the tab — and its + /// captures — alive. Safe to call for an unknown / fleet-only connection (no-op). /// - public IReadOnlyList RemoveConnection(string connectionId) + public void RemoveConnection(string connectionId) { lock (_gate) { - if (!_byConnection.Remove(connectionId, out var set)) - return Array.Empty(); + if (!_connToTab.Remove(connectionId, out var tabId)) + return; + if (!_tabs.TryGetValue(tabId, out var tab)) + return; - var dropped = new List(); - foreach (var plcName in set) + tab.Connections.Remove(connectionId); + if (tab.Connections.Count > 0) + return; // the tab is still alive on another connection + + _tabs.Remove(tabId); + foreach (var plcName in tab.Plcs) { - int count = _plcCounts.GetValueOrDefault(plcName); + int count = _plcViewerTabs.GetValueOrDefault(plcName); if (count <= 1) - { - _plcCounts.Remove(plcName); - dropped.Add(plcName); - } + _plcViewerTabs.Remove(plcName); else - { - _plcCounts[plcName] = count - 1; - } + _plcViewerTabs[plcName] = count - 1; } - return dropped; } } - /// PLC names that currently have at least one detail-page subscriber. + /// PLC names that currently have at least one detail-page tab open. public IReadOnlyList ActivePlcs() { lock (_gate) { - return _plcCounts.Count == 0 ? Array.Empty() : _plcCounts.Keys.ToArray(); + return _plcViewerTabs.Count == 0 + ? Array.Empty() + : _plcViewerTabs.Keys.ToArray(); } } } diff --git a/mbproxy/src/Mbproxy/Admin/StatusBroadcaster.cs b/mbproxy/src/Mbproxy/Admin/StatusBroadcaster.cs index 00d8977..e60cf98 100644 --- a/mbproxy/src/Mbproxy/Admin/StatusBroadcaster.cs +++ b/mbproxy/src/Mbproxy/Admin/StatusBroadcaster.cs @@ -94,7 +94,13 @@ internal sealed class StatusBroadcaster : IAsyncDisposable _logger.LogError(ex, "StatusBroadcaster: fleet push failed"); } - foreach (var plcName in _tracker.ActivePlcs()) + // Reconcile capture arm state from the live viewer set. This is the single + // arm/disarm authority — doing it here (one thread, every cycle) means a SignalR + // reconnect or a hot-reload capture rebuild can never strand a capture armed. + var activePlcs = _tracker.ActivePlcs(); + _captureRegistry.ReconcileArmed(activePlcs); + + foreach (var plcName in activePlcs) { try { diff --git a/mbproxy/src/Mbproxy/Admin/StatusHub.cs b/mbproxy/src/Mbproxy/Admin/StatusHub.cs index e68682c..4690a58 100644 --- a/mbproxy/src/Mbproxy/Admin/StatusHub.cs +++ b/mbproxy/src/Mbproxy/Admin/StatusHub.cs @@ -1,4 +1,3 @@ -using Mbproxy.Proxy; using Microsoft.AspNetCore.SignalR; namespace Mbproxy.Admin; @@ -10,15 +9,18 @@ namespace Mbproxy.Admin; /// group and receives a "fleet" message every /// push tick. /// — a connection-detail page (GET /plc/{name}) -/// joins and receives a "plc" message. The first -/// subscriber to a PLC arms that PLC's tag-value capture; the last to leave -/// disarms it (on-demand capture). +/// joins and receives a "plc" message. It also +/// registers the calling tab with the so +/// the PLC's tag-value capture is armed while the page is open. /// /// -/// The hub itself is transient (one instance per call). Cross-call state — the -/// subscriber counts that drive capture arming — lives in the singleton -/// . The actual pushes are issued by -/// , not the hub. +/// The hub itself is transient (one instance per call). Cross-call state — which +/// tabs view which PLCs — lives in the singleton . +/// The hub deliberately does not arm or disarm captures: a SignalR reconnect +/// gives the connection a new ConnectionId, and arming off that lifetime leaks. +/// Instead the hub only mutates the tracker (keyed on a stable client tabId) and +/// reconciles capture arm state from the tracker each +/// push cycle. The actual pushes are issued by the broadcaster, not the hub. /// internal sealed class StatusHub : Hub { @@ -29,39 +31,36 @@ internal sealed class StatusHub : Hub public static string PlcGroup(string plcName) => "plc:" + plcName; private readonly PlcSubscriptionTracker _tracker; - private readonly TagCaptureRegistry _captureRegistry; - public StatusHub(PlcSubscriptionTracker tracker, TagCaptureRegistry captureRegistry) - { - _tracker = tracker; - _captureRegistry = captureRegistry; - } + public StatusHub(PlcSubscriptionTracker tracker) => _tracker = tracker; /// Subscribes the calling connection to fleet-wide status pushes. public Task SubscribeFleet() => Groups.AddToGroupAsync(Context.ConnectionId, FleetGroup); /// - /// Subscribes the calling connection to one PLC's detail pushes and arms that PLC's - /// tag-value capture if this is its first viewer. + /// Subscribes the calling connection to one PLC's detail pushes. + /// is a stable per-page-load identifier supplied by the client so a transport + /// reconnect (which changes ConnectionId) is recognised as the same viewer. /// - public async Task SubscribePlc(string plcName) + public async Task SubscribePlc(string plcName, string tabId) { + // Register with the tracker first (synchronous, lock-guarded) so this connection's + // own OnDisconnectedAsync — which SignalR dispatches only after this invocation's + // Task completes — always observes a consistent state. Capture arming is NOT done + // here; StatusBroadcaster reconciles it each cycle from the tracker. + _tracker.SubscribePlc(Context.ConnectionId, tabId, plcName); await Groups.AddToGroupAsync(Context.ConnectionId, PlcGroup(plcName)).ConfigureAwait(false); - - if (_tracker.Add(Context.ConnectionId, plcName)) - _captureRegistry.Arm(plcName); // no-op for an unknown PLC name } /// - /// On disconnect, drops every subscription the connection held and disarms the - /// capture of any PLC whose last viewer just left. + /// On disconnect, releases the connection from its tab. If it was the tab's last + /// connection the tab's PLC subscriptions are dropped; the broadcaster disarms the + /// now-unviewed captures on its next cycle. /// public override Task OnDisconnectedAsync(Exception? exception) { - foreach (var plcName in _tracker.RemoveConnection(Context.ConnectionId)) - _captureRegistry.Disarm(plcName); - + _tracker.RemoveConnection(Context.ConnectionId); return base.OnDisconnectedAsync(exception); } } diff --git a/mbproxy/src/Mbproxy/Admin/wwwroot/dashboard.js b/mbproxy/src/Mbproxy/Admin/wwwroot/dashboard.js index 9eb2611..0baebc0 100644 --- a/mbproxy/src/Mbproxy/Admin/wwwroot/dashboard.js +++ b/mbproxy/src/Mbproxy/Admin/wwwroot/dashboard.js @@ -237,30 +237,37 @@ } // ── SignalR ──────────────────────────────────────────────────────────── - function connect() { - const connection = new signalR.HubConnectionBuilder() - .withUrl('/hub/status') - .withAutomaticReconnect([0, 1000, 2000, 5000, 10000]) - .build(); + const connection = new signalR.HubConnectionBuilder() + .withUrl('/hub/status') + .withAutomaticReconnect([0, 1000, 2000, 5000, 10000]) + .build(); - connection.on('fleet', onSnapshot); + connection.on('fleet', onSnapshot); + connection.onreconnecting(() => setConn('connecting', 'reconnecting')); + connection.onreconnected(() => { + setConn('connected'); + connection.invoke('SubscribeFleet').catch(() => {}); + }); + connection.onclose(() => setConn('disconnected', 'disconnected')); - connection.onreconnecting(() => setConn('connecting', 'reconnecting')); - connection.onreconnected(() => { setConn('connected'); connection.invoke('SubscribeFleet'); }); - connection.onclose(() => setConn('disconnected', 'disconnected')); - - async function start() { - try { - setConn('connecting', 'connecting'); + // Cold start. withAutomaticReconnect only recovers an already-established + // connection, so the initial connect needs its own retry: capped exponential + // backoff, and start() only when the socket is actually Disconnected so a + // subscribe-only failure never tries to re-start a live connection. + let retryMs = 1000; + async function connect() { + setConn('connecting', 'connecting'); + try { + if (connection.state === signalR.HubConnectionState.Disconnected) await connection.start(); - await connection.invoke('SubscribeFleet'); - setConn('connected'); - } catch { - setConn('disconnected', 'retrying'); - setTimeout(start, 3000); - } + await connection.invoke('SubscribeFleet'); + setConn('connected'); + retryMs = 1000; + } catch { + setConn('disconnected', 'retrying'); + setTimeout(connect, retryMs); + retryMs = Math.min(retryMs * 2, 30000); } - start(); } // ── Boot ─────────────────────────────────────────────────────────────── diff --git a/mbproxy/src/Mbproxy/Admin/wwwroot/detail.js b/mbproxy/src/Mbproxy/Admin/wwwroot/detail.js index 7b50903..539f5ed 100644 --- a/mbproxy/src/Mbproxy/Admin/wwwroot/detail.js +++ b/mbproxy/src/Mbproxy/Admin/wwwroot/detail.js @@ -175,10 +175,16 @@ return Math.round((100 * hit) / total) + '%'; } - // ── Render: PLC removed by hot-reload ────────────────────────────────── - function renderMissing() { - $('notice').hidden = false; + // ── Notices: PLC removed by hot-reload, or unknown / unreachable ─────── + function showNotice(msg) { + const n = $('notice'); + n.textContent = msg; + n.hidden = false; $('cards').hidden = true; + } + function renderMissing() { + showNotice('This PLC is no longer in the configuration — it was likely ' + + 'removed by a hot-reload. Counters and the debug view are unavailable.'); $('cards').innerHTML = ''; $('plc-sub').textContent = 'not configured'; $('plc-state').innerHTML = 'removed'; @@ -220,6 +226,7 @@ // ── Snapshot handler ─────────────────────────────────────────────────── function onDetail(detail) { + gotSnapshot(); if (detail.plc) renderPlc(detail.plc); else renderMissing(); renderDebug(detail.debug || { captureArmed: false, tags: [] }); @@ -231,31 +238,65 @@ $('conn-text').textContent = text || state; } + // ── Unknown-PLC watchdog ─────────────────────────────────────────────── + // SubscribePlc succeeds for any name; an unconfigured PLC simply never + // produces a 'plc' push. If no snapshot lands shortly after connecting, + // say so instead of sitting on "Waiting for first snapshot…" forever. + let firstSnapshotTimer = null; + function armSnapshotWatchdog() { + clearTimeout(firstSnapshotTimer); + firstSnapshotTimer = setTimeout(() => { + showNotice(`No data for "${plcName}". This PLC is not in the proxy ` + + `configuration, or the admin feed is not delivering — ` + + `check the name against the fleet page.`); + }, 6000); + } + function gotSnapshot() { + clearTimeout(firstSnapshotTimer); + firstSnapshotTimer = null; + } + // ── SignalR ──────────────────────────────────────────────────────────── - function connect() { - const connection = new signalR.HubConnectionBuilder() - .withUrl('/hub/status') - .withAutomaticReconnect([0, 1000, 2000, 5000, 10000]) - .build(); + // Stable per-page-load id so a transport reconnect (which assigns a fresh + // ConnectionId) is recognised server-side as the same viewer — that is what + // keeps the PLC's tag-value capture from leaking armed. Math.random, not + // crypto.randomUUID: the dashboard is served over plain http on the LAN, + // where randomUUID is unavailable (non-secure context). + const tabId = 't-' + Date.now().toString(36) + '-' + + Math.random().toString(36).slice(2, 10); - connection.on('plc', onDetail); + const connection = new signalR.HubConnectionBuilder() + .withUrl('/hub/status') + .withAutomaticReconnect([0, 1000, 2000, 5000, 10000]) + .build(); - connection.onreconnecting(() => setConn('connecting', 'reconnecting')); - connection.onreconnected(() => { setConn('connected'); connection.invoke('SubscribePlc', plcName); }); - connection.onclose(() => setConn('disconnected', 'disconnected')); + connection.on('plc', onDetail); + connection.onreconnecting(() => setConn('connecting', 'reconnecting')); + connection.onreconnected(() => { + setConn('connected'); + connection.invoke('SubscribePlc', plcName, tabId).catch(() => {}); + }); + connection.onclose(() => setConn('disconnected', 'disconnected')); - async function start() { - try { - setConn('connecting', 'connecting'); + // Cold start. withAutomaticReconnect only recovers an already-established + // connection, so the initial connect needs its own retry: capped exponential + // backoff, and start() only when the socket is actually Disconnected so a + // subscribe-only failure never tries to re-start a live connection. + let retryMs = 1000; + async function connect() { + setConn('connecting', 'connecting'); + try { + if (connection.state === signalR.HubConnectionState.Disconnected) await connection.start(); - await connection.invoke('SubscribePlc', plcName); - setConn('connected'); - } catch { - setConn('disconnected', 'retrying'); - setTimeout(start, 3000); - } + await connection.invoke('SubscribePlc', plcName, tabId); + setConn('connected'); + armSnapshotWatchdog(); + retryMs = 1000; + } catch { + setConn('disconnected', 'retrying'); + setTimeout(connect, retryMs); + retryMs = Math.min(retryMs * 2, 30000); } - start(); } document.addEventListener('DOMContentLoaded', connect); diff --git a/mbproxy/src/Mbproxy/Proxy/TagCaptureRegistry.cs b/mbproxy/src/Mbproxy/Proxy/TagCaptureRegistry.cs index 635b783..6590bab 100644 --- a/mbproxy/src/Mbproxy/Proxy/TagCaptureRegistry.cs +++ b/mbproxy/src/Mbproxy/Proxy/TagCaptureRegistry.cs @@ -11,9 +11,10 @@ namespace Mbproxy.Proxy; /// Registered as a DI singleton. and /// call as they /// build each ; -/// calls for hot-reload-removed PLCs. The admin -/// StatusHub / StatusBroadcaster call / -/// / / . +/// calls for hot-reload-removed PLCs. StatusBroadcaster +/// calls every push cycle (the single arm/disarm authority) +/// and on shutdown; StatusSnapshotBuilder calls +/// . /// internal sealed class TagCaptureRegistry { @@ -23,20 +24,17 @@ internal sealed class TagCaptureRegistry /// /// Returns the capture for , creating it on first call. /// A subsequent call (hot-reload reseat/restart, where the tag set may have changed) - /// rebuilds the capture for 's current tags, preserving the - /// armed flag so an open detail page keeps capturing across the reload. + /// rebuilds the capture for 's current tags. The rebuilt + /// capture is disarmed; re-arms it on its next push + /// cycle (within one AdminPushIntervalMs) if the PLC still has a viewer — so + /// arm state is never carried across the rebuild, which removes any race between + /// arming and the rebuild. /// public TagValueCapture GetOrCreate(string plcName, BcdTagMap map) => _captures.AddOrUpdate( plcName, _ => new TagValueCapture(map.All), - (_, existing) => - { - var rebuilt = new TagValueCapture(map.All); - if (existing.IsArmed) - rebuilt.Arm(); - return rebuilt; - }); + (_, _) => new TagValueCapture(map.All)); /// Drops the capture for a hot-reload-removed PLC. public void Remove(string plcName) => _captures.TryRemove(plcName, out _); @@ -45,18 +43,28 @@ internal sealed class TagCaptureRegistry public bool TryGet(string plcName, out TagValueCapture capture) => _captures.TryGetValue(plcName, out capture!); - /// Arms a PLC's capture. No-op for an unknown PLC name. - public void Arm(string plcName) + /// + /// Reconciles every capture's armed state against — + /// the set of PLCs that currently have a detail-page viewer. Captures for active + /// PLCs are armed, all others disarmed. Called once per push cycle by + /// , so it is the single arm/disarm authority: + /// no hub thread ever arms a capture, which both removes the race against a + /// hot-reload and makes a leaked subscriber impossible to + /// reach here (the tracker is reconnect-safe). + /// + public void ReconcileArmed(IReadOnlyCollection activePlcs) { - if (_captures.TryGetValue(plcName, out var c)) - c.Arm(); - } + var active = activePlcs as IReadOnlySet + ?? activePlcs.ToHashSet(StringComparer.Ordinal); - /// Disarms a PLC's capture. No-op for an unknown PLC name. - public void Disarm(string plcName) - { - if (_captures.TryGetValue(plcName, out var c)) - c.Disarm(); + foreach (var (name, capture) in _captures) + { + bool shouldArm = active.Contains(name); + if (shouldArm && !capture.IsArmed) + capture.Arm(); + else if (!shouldArm && capture.IsArmed) + capture.Disarm(); + } } /// diff --git a/mbproxy/src/Mbproxy/Proxy/TagValueCapture.cs b/mbproxy/src/Mbproxy/Proxy/TagValueCapture.cs index 4e4bb28..da8c4e1 100644 --- a/mbproxy/src/Mbproxy/Proxy/TagValueCapture.cs +++ b/mbproxy/src/Mbproxy/Proxy/TagValueCapture.cs @@ -131,6 +131,15 @@ internal sealed class TagValueCapture new TagValueObservation( _addresses[idx], _widths[idx], _names[idx], rawLow, rawHigh, decoded, direction, DateTimeOffset.UtcNow)); + + // A concurrent Disarm() may have flipped _armed (and cleared the slots) between + // the _armed check above and the write just made — which would strand a stale + // observation on a disarmed capture, defeating the "reopened page shows no stale + // data" contract. Re-read _armed: if it is now false, Disarm has either already + // run (so this write must be undone) or is still running (its own slot-clear + // pass will null this slot). Either way, null it here to be safe. + if (!_armed) + Volatile.Write(ref _slots[idx], null); } /// diff --git a/mbproxy/tests/Mbproxy.Tests/Admin/DebugDtoSerializationTests.cs b/mbproxy/tests/Mbproxy.Tests/Admin/DebugDtoSerializationTests.cs new file mode 100644 index 0000000..ece8f3a --- /dev/null +++ b/mbproxy/tests/Mbproxy.Tests/Admin/DebugDtoSerializationTests.cs @@ -0,0 +1,43 @@ +using System.Text.Json; +using Mbproxy.Admin; +using Shouldly; +using Xunit; + +namespace Mbproxy.Tests.Admin; + +/// +/// Locks the SignalR payload wire shape. The hub serialises detail / fleet payloads +/// with a camelCase property policy (see AdminEndpointHost's AddJsonProtocol), +/// and the dashboard JS reads camelCase field names — so a regression to the naming +/// policy would silently break every field on the live feed with no other failing test. +/// +[Trait("Category", "Unit")] +public sealed class DebugDtoSerializationTests +{ + // The exact policy AdminEndpointHost configures on the hub's PayloadSerializerOptions. + private static readonly JsonSerializerOptions HubOptions = + new() { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; + + [Fact] + public void PlcDetailResponse_SerializesWithCamelCaseFieldNames() + { + var detail = new PlcDetailResponse( + Plc: null, + Debug: new PlcDebugSnapshot( + CaptureArmed: true, + Tags: [new TagValueDto( + Address: 100, Width: 16, Name: "Left AirSP", HasValue: true, + Direction: "read", RawHex: "0x1234", DecodedValue: 1234, + UpdatedAtUtc: "2026-05-16T00:00:00Z", AgeSeconds: 1.5)])); + + string json = JsonSerializer.Serialize(detail, HubOptions); + + // Case.Sensitive throughout — Shouldly's string contains defaults to + // case-insensitive, which would not distinguish camelCase from PascalCase. + json.ShouldContain("\"captureArmed\"", Case.Sensitive); + json.ShouldContain("\"decodedValue\"", Case.Sensitive); + json.ShouldContain("\"updatedAtUtc\"", Case.Sensitive); + json.ShouldNotContain("\"CaptureArmed\"", Case.Sensitive); + json.ShouldNotContain("\"DecodedValue\"", Case.Sensitive); + } +} diff --git a/mbproxy/tests/Mbproxy.Tests/Admin/PlcSubscriptionTrackerTests.cs b/mbproxy/tests/Mbproxy.Tests/Admin/PlcSubscriptionTrackerTests.cs new file mode 100644 index 0000000..2b59edc --- /dev/null +++ b/mbproxy/tests/Mbproxy.Tests/Admin/PlcSubscriptionTrackerTests.cs @@ -0,0 +1,110 @@ +using Mbproxy.Admin; +using Shouldly; +using Xunit; + +namespace Mbproxy.Tests.Admin; + +/// +/// Unit tests for — the tab-keyed, reconnect-safe +/// record of which PLC detail pages are open. Includes a concurrency stress test, since +/// the tracker is mutated from multiple SignalR hub-dispatch threads. +/// +[Trait("Category", "Unit")] +public sealed class PlcSubscriptionTrackerTests +{ + [Fact] + public void Subscribe_ThenRemoveLastConnection_ClearsViewer() + { + var t = new PlcSubscriptionTracker(); + t.SubscribePlc("c1", "tab", "plc"); + t.ActivePlcs().ShouldBe(["plc"]); + + t.RemoveConnection("c1"); + t.ActivePlcs().ShouldBeEmpty(); + } + + [Fact] + public void SameTab_TwoConnections_StaysActiveUntilLastConnectionGone() + { + // Reconnect overlap: the same tab briefly holds two connections. Dropping the + // old one must not release the tab — this is the leak C2 guards against. + var t = new PlcSubscriptionTracker(); + t.SubscribePlc("c-old", "tab", "plc"); + t.SubscribePlc("c-new", "tab", "plc"); + + t.RemoveConnection("c-old"); + t.ActivePlcs().ShouldContain("plc", "the tab is still alive on the second connection"); + + t.RemoveConnection("c-new"); + t.ActivePlcs().ShouldBeEmpty(); + } + + [Fact] + public void DistinctTabs_AreCountedSeparately() + { + var t = new PlcSubscriptionTracker(); + t.SubscribePlc("c1", "tab-A", "plc"); + t.SubscribePlc("c2", "tab-B", "plc"); + + t.RemoveConnection("c1"); + t.ActivePlcs().ShouldContain("plc", "the second tab still views the PLC"); + + t.RemoveConnection("c2"); + t.ActivePlcs().ShouldBeEmpty(); + } + + [Fact] + public void RepeatedSubscribe_SameTabSamePlc_IsIdempotent() + { + var t = new PlcSubscriptionTracker(); + t.SubscribePlc("c1", "tab", "plc"); + t.SubscribePlc("c1", "tab", "plc"); // redundant repeat + t.ActivePlcs().ShouldBe(["plc"]); + + t.RemoveConnection("c1"); + t.ActivePlcs().ShouldBeEmpty("a repeated subscribe must not inflate the viewer count"); + } + + [Fact] + public void OneConnection_MultiplePlcs_AllReleasedTogether() + { + var t = new PlcSubscriptionTracker(); + t.SubscribePlc("c1", "tab", "plc-a"); + t.SubscribePlc("c1", "tab", "plc-b"); + t.ActivePlcs().Count.ShouldBe(2); + + t.RemoveConnection("c1"); + t.ActivePlcs().ShouldBeEmpty(); + } + + [Fact] + public void RemoveConnection_Unknown_IsNoOp() + { + var t = new PlcSubscriptionTracker(); + + Should.NotThrow(() => t.RemoveConnection("never-seen")); + t.ActivePlcs().ShouldBeEmpty(); + } + + [Fact] + public async Task ConcurrentSubscribeAndRemove_NeverLeaksOrThrows() + { + var t = new PlcSubscriptionTracker(); + const int tasks = 16; + const int iterations = 5_000; + + await Task.WhenAll(Enumerable.Range(0, tasks).Select(taskNo => Task.Run(() => + { + for (int i = 0; i < iterations; i++) + { + string conn = $"c{taskNo}-{i}"; + string tab = $"tab{taskNo}-{i}"; + t.SubscribePlc(conn, tab, "plc"); + t.RemoveConnection(conn); + } + }, TestContext.Current.CancellationToken))); + + t.ActivePlcs().ShouldBeEmpty( + "every subscribe was paired with a remove — no viewer count may leak"); + } +} diff --git a/mbproxy/tests/Mbproxy.Tests/Admin/StatusBroadcasterTests.cs b/mbproxy/tests/Mbproxy.Tests/Admin/StatusBroadcasterTests.cs index 3dfd379..d6f70fc 100644 --- a/mbproxy/tests/Mbproxy.Tests/Admin/StatusBroadcasterTests.cs +++ b/mbproxy/tests/Mbproxy.Tests/Admin/StatusBroadcasterTests.cs @@ -45,6 +45,8 @@ public sealed class StatusBroadcasterTests hostBuilder.Configuration.AddInMemoryCollection(new Dictionary { ["Mbproxy:AdminPort"] = "0", + // Fast tick so the LoopAsync test observes several cycles quickly. + ["Mbproxy:AdminPushIntervalMs"] = "100", }); hostBuilder.Services.AddSerilog( new LoggerConfiguration().MinimumLevel.Fatal().CreateLogger(), dispose: false); @@ -96,7 +98,7 @@ public sealed class StatusBroadcasterTests { await using var h = await BuildAsync(); h.Registry.GetOrCreate("plc-x", BcdTagMap.Empty); - h.Tracker.Add("conn-1", "plc-x"); + h.Tracker.SubscribePlc("conn-1", "tab-1", "plc-x"); await h.Broadcaster.PushOnceAsync(TestContext.Current.CancellationToken); @@ -105,16 +107,60 @@ public sealed class StatusBroadcasterTests push.Detail.Debug.ShouldNotBeNull(); } + [Fact] + public async Task PushOnce_ReconcilesCaptureArmState_FromActiveViewers() + { + await using var h = await BuildAsync(); + h.Registry.GetOrCreate("plc-x", BcdTagMap.Empty); + + // No viewer yet — a push must leave the capture disarmed. + await h.Broadcaster.PushOnceAsync(TestContext.Current.CancellationToken); + h.Registry.TryGet("plc-x", out var capture).ShouldBeTrue(); + capture.IsArmed.ShouldBeFalse("no detail page open — capture stays disarmed"); + + // A viewer opens the detail page — the next push arms the capture. + h.Tracker.SubscribePlc("conn-1", "tab-1", "plc-x"); + await h.Broadcaster.PushOnceAsync(TestContext.Current.CancellationToken); + capture.IsArmed.ShouldBeTrue("the broadcaster reconciles the capture armed for a viewed PLC"); + + // The viewer leaves — the next push disarms it again. + h.Tracker.RemoveConnection("conn-1"); + await h.Broadcaster.PushOnceAsync(TestContext.Current.CancellationToken); + capture.IsArmed.ShouldBeFalse("the broadcaster disarms a capture once its last viewer leaves"); + } + [Fact] public async Task StopAsync_DisarmsEveryCapture() { await using var h = await BuildAsync(); h.Registry.GetOrCreate("plc-x", BcdTagMap.Empty); - h.Registry.Arm("plc-x"); + h.Registry.ReconcileArmed(["plc-x"]); await h.Broadcaster.StopAsync(); h.Registry.TryGet("plc-x", out var capture).ShouldBeTrue(); capture.IsArmed.ShouldBeFalse(); } + + [Fact] + public async Task Loop_PushesRepeatedly_ThenStopsAfterStopAsync() + { + await using var h = await BuildAsync(); + + h.Broadcaster.Start(); + + // The harness runs at AdminPushIntervalMs = 100 ms; wait (generously) for the + // background loop to complete several cycles. + var deadline = DateTime.UtcNow.AddSeconds(10); + while (h.Sink.FleetPushes.Count < 3 && DateTime.UtcNow < deadline) + await Task.Delay(50, TestContext.Current.CancellationToken); + + h.Sink.FleetPushes.Count.ShouldBeGreaterThanOrEqualTo(3, + "the background loop must push the fleet snapshot every interval"); + + await h.Broadcaster.StopAsync(); + int afterStop = h.Sink.FleetPushes.Count; + await Task.Delay(400, TestContext.Current.CancellationToken); + h.Sink.FleetPushes.Count.ShouldBe(afterStop, "no pushes may occur after StopAsync"); + } } diff --git a/mbproxy/tests/Mbproxy.Tests/Admin/StatusHubTests.cs b/mbproxy/tests/Mbproxy.Tests/Admin/StatusHubTests.cs index 2bf58b6..62d332d 100644 --- a/mbproxy/tests/Mbproxy.Tests/Admin/StatusHubTests.cs +++ b/mbproxy/tests/Mbproxy.Tests/Admin/StatusHubTests.cs @@ -1,27 +1,23 @@ using Mbproxy.Admin; -using Mbproxy.Bcd; -using Mbproxy.Proxy; using Shouldly; using Xunit; namespace Mbproxy.Tests.Admin; /// -/// Unit tests for — group joins and on-demand capture -/// arming. Uses hand-written SignalR test doubles (see ); -/// no SignalR host is started. +/// Unit tests for — group joins and subscription tracking. +/// Capture arming is the broadcaster's job; the hub only mutates the +/// . Uses hand-written SignalR test doubles +/// (see ); no SignalR host is started. /// [Trait("Category", "Unit")] public sealed class StatusHubTests { private static StatusHub MakeHub( - string connectionId, - PlcSubscriptionTracker tracker, - TagCaptureRegistry registry, - out FakeGroupManager groups) + string connectionId, PlcSubscriptionTracker tracker, out FakeGroupManager groups) { groups = new FakeGroupManager(); - return new StatusHub(tracker, registry) + return new StatusHub(tracker) { Context = new FakeHubCallerContext(connectionId), Groups = groups, @@ -31,7 +27,7 @@ public sealed class StatusHubTests [Fact] public async Task SubscribeFleet_JoinsFleetGroup() { - var hub = MakeHub("conn-1", new PlcSubscriptionTracker(), new TagCaptureRegistry(), out var groups); + var hub = MakeHub("conn-1", new PlcSubscriptionTracker(), out var groups); await hub.SubscribeFleet(); @@ -39,53 +35,63 @@ public sealed class StatusHubTests } [Fact] - public async Task SubscribePlc_JoinsPlcGroup_AndArmsCapture() + public async Task SubscribePlc_JoinsPlcGroup_AndTracksViewer() { - var registry = new TagCaptureRegistry(); - registry.GetOrCreate("plc-1", BcdTagMap.Empty); - var hub = MakeHub("conn-1", new PlcSubscriptionTracker(), registry, out var groups); + var tracker = new PlcSubscriptionTracker(); + var hub = MakeHub("conn-1", tracker, out var groups); - await hub.SubscribePlc("plc-1"); + await hub.SubscribePlc("plc-1", "tab-A"); groups.Added.ShouldContain(("conn-1", StatusHub.PlcGroup("plc-1"))); - registry.TryGet("plc-1", out var capture).ShouldBeTrue(); - capture.IsArmed.ShouldBeTrue(); + tracker.ActivePlcs().ShouldContain("plc-1"); } [Fact] - public async Task SecondSubscriber_FirstLeaveKeepsArmed_LastLeaveDisarms() + public async Task Reconnect_SameTab_NewConnection_DoesNotLeakViewer() { - var tracker = new PlcSubscriptionTracker(); - var registry = new TagCaptureRegistry(); - registry.GetOrCreate("plc-1", BcdTagMap.Empty); + // A transport reconnect: the same browser tab acquires a new ConnectionId and + // re-subscribes; the old connection's OnDisconnectedAsync then fires late. The + // PLC must not be left with a stranded viewer once the tab finally closes. + var tracker = new PlcSubscriptionTracker(); - var hub1 = MakeHub("conn-1", tracker, registry, out _); - var hub2 = MakeHub("conn-2", tracker, registry, out _); + var first = MakeHub("conn-old", tracker, out _); + await first.SubscribePlc("plc-1", "tab-A"); - await hub1.SubscribePlc("plc-1"); - await hub2.SubscribePlc("plc-1"); + var second = MakeHub("conn-new", tracker, out _); + await second.SubscribePlc("plc-1", "tab-A"); - registry.TryGet("plc-1", out var capture).ShouldBeTrue(); - capture.IsArmed.ShouldBeTrue(); + await first.OnDisconnectedAsync(null); // late disconnect of the old connection + tracker.ActivePlcs().ShouldContain("plc-1", + "the tab is still open on the reconnected connection"); - // First viewer leaves — a second viewer remains, so capture stays armed. - await hub1.OnDisconnectedAsync(null); - capture.IsArmed.ShouldBeTrue("capture must stay armed while another detail page is open"); - - // Last viewer leaves — capture disarms. - await hub2.OnDisconnectedAsync(null); - capture.IsArmed.ShouldBeFalse("capture must disarm when the last viewer leaves"); + await second.OnDisconnectedAsync(null); // the tab finally closes + tracker.ActivePlcs().ShouldBeEmpty("no viewer may be stranded after the tab closes"); } [Fact] - public async Task SubscribePlc_UnknownPlc_DoesNotThrow_AndArmsNothing() + public async Task TwoTabs_FirstCloseKeepsActive_LastCloseClears() { - var registry = new TagCaptureRegistry(); // no captures registered - var hub = MakeHub("conn-1", new PlcSubscriptionTracker(), registry, out var groups); + var tracker = new PlcSubscriptionTracker(); - await Should.NotThrowAsync(async () => await hub.SubscribePlc("ghost")); + var tabA = MakeHub("conn-a", tracker, out _); + var tabB = MakeHub("conn-b", tracker, out _); + await tabA.SubscribePlc("plc-1", "tab-A"); + await tabB.SubscribePlc("plc-1", "tab-B"); + + await tabA.OnDisconnectedAsync(null); + tracker.ActivePlcs().ShouldContain("plc-1", "a second tab is still viewing the PLC"); + + await tabB.OnDisconnectedAsync(null); + tracker.ActivePlcs().ShouldBeEmpty(); + } + + [Fact] + public async Task SubscribePlc_UnknownPlc_DoesNotThrow() + { + var hub = MakeHub("conn-1", new PlcSubscriptionTracker(), out var groups); + + await Should.NotThrowAsync(async () => await hub.SubscribePlc("ghost", "tab-A")); groups.Added.ShouldContain(("conn-1", StatusHub.PlcGroup("ghost"))); - registry.TryGet("ghost", out _).ShouldBeFalse(); } } diff --git a/mbproxy/tests/Mbproxy.Tests/Proxy/TagCaptureRegistryTests.cs b/mbproxy/tests/Mbproxy.Tests/Proxy/TagCaptureRegistryTests.cs index 5b0d379..609dc00 100644 --- a/mbproxy/tests/Mbproxy.Tests/Proxy/TagCaptureRegistryTests.cs +++ b/mbproxy/tests/Mbproxy.Tests/Proxy/TagCaptureRegistryTests.cs @@ -7,8 +7,9 @@ using Xunit; namespace Mbproxy.Tests.Proxy; /// -/// Unit tests for — the shared seam that arms and -/// disarms per-PLC instances. +/// Unit tests for — the shared seam holding per-PLC +/// instances. Arm state is reconciled in bulk against the +/// live viewer set (not toggled per PLC) so the broadcaster is the single authority. /// [Trait("Category", "Unit")] public sealed class TagCaptureRegistryTests @@ -25,48 +26,69 @@ public sealed class TagCaptureRegistryTests } [Fact] - public void GetOrCreate_ReturnsSameInstance_OnRepeatCall_WhenTagSetUnchanged() + public void GetOrCreate_ReturnsLiveInstance_OnRepeatCall() { var registry = new TagCaptureRegistry(); - var first = registry.GetOrCreate("plc-1", Map((100, 16))); + registry.GetOrCreate("plc-1", Map((100, 16))); var second = registry.GetOrCreate("plc-1", Map((100, 16))); - // AddOrUpdate's update path rebuilds; both must be live and consistent. second.TagCount.ShouldBe(1); registry.TryGet("plc-1", out var current).ShouldBeTrue(); current.ShouldBeSameAs(second); } [Fact] - public void GetOrCreate_Rebuild_PreservesArmedFlag() + public void GetOrCreate_Rebuild_ProducesDisarmedCapture_AndReconcileReArms() { + // The rebuilt capture is intentionally disarmed: ReconcileArmed re-arms it within + // one push cycle if the PLC still has a viewer, so arm state is never carried + // across the rebuild — which removes any arm-vs-rebuild race. var registry = new TagCaptureRegistry(); - var capture = registry.GetOrCreate("plc-1", Map((100, 16))); - capture.Arm(); + registry.GetOrCreate("plc-1", Map((100, 16))); + registry.ReconcileArmed(["plc-1"]); + registry.TryGet("plc-1", out var armed).ShouldBeTrue(); + armed.IsArmed.ShouldBeTrue(); // Hot-reload reseat: same PLC, changed tag set. var rebuilt = registry.GetOrCreate("plc-1", Map((100, 16), (200, 32))); - - rebuilt.ShouldNotBeSameAs(capture); - rebuilt.IsArmed.ShouldBeTrue("a rebuilt capture must keep capturing for an open detail page"); + rebuilt.ShouldNotBeSameAs(armed); + rebuilt.IsArmed.ShouldBeFalse("a rebuilt capture starts disarmed"); rebuilt.TagCount.ShouldBe(2); + + // The next reconcile re-arms it because the PLC is still viewed. + registry.ReconcileArmed(["plc-1"]); + rebuilt.IsArmed.ShouldBeTrue(); } [Fact] - public void Arm_And_Disarm_ReachTheRightCapture() + public void ReconcileArmed_ArmsActivePlcs_DisarmsTheRest() { var registry = new TagCaptureRegistry(); registry.GetOrCreate("plc-1", Map((100, 16))); registry.GetOrCreate("plc-2", Map((100, 16))); - registry.Arm("plc-1"); - + registry.ReconcileArmed(["plc-1"]); registry.TryGet("plc-1", out var c1).ShouldBeTrue(); registry.TryGet("plc-2", out var c2).ShouldBeTrue(); c1.IsArmed.ShouldBeTrue(); c2.IsArmed.ShouldBeFalse(); - registry.Disarm("plc-1"); + // plc-1's viewer leaves, plc-2 gains one. + registry.ReconcileArmed(["plc-2"]); + c1.IsArmed.ShouldBeFalse(); + c2.IsArmed.ShouldBeTrue(); + } + + [Fact] + public void ReconcileArmed_EmptyActiveSet_DisarmsEverything() + { + var registry = new TagCaptureRegistry(); + registry.GetOrCreate("plc-1", Map((100, 16))); + registry.ReconcileArmed(["plc-1"]); + + registry.ReconcileArmed(Array.Empty()); + + registry.TryGet("plc-1", out var c1).ShouldBeTrue(); c1.IsArmed.ShouldBeFalse(); } @@ -76,8 +98,7 @@ public sealed class TagCaptureRegistryTests var registry = new TagCaptureRegistry(); registry.GetOrCreate("plc-1", Map((100, 16))); registry.GetOrCreate("plc-2", Map((100, 16))); - registry.Arm("plc-1"); - registry.Arm("plc-2"); + registry.ReconcileArmed(["plc-1", "plc-2"]); registry.DisarmAll(); @@ -92,8 +113,7 @@ public sealed class TagCaptureRegistryTests { var registry = new TagCaptureRegistry(); - Should.NotThrow(() => registry.Arm("ghost")); - Should.NotThrow(() => registry.Disarm("ghost")); + Should.NotThrow(() => registry.ReconcileArmed(["ghost"])); Should.NotThrow(() => registry.Remove("ghost")); registry.TryGet("ghost", out _).ShouldBeFalse(); } diff --git a/mbproxy/tests/Mbproxy.Tests/Proxy/TagValueCaptureTests.cs b/mbproxy/tests/Mbproxy.Tests/Proxy/TagValueCaptureTests.cs index 20b8040..d52da6f 100644 --- a/mbproxy/tests/Mbproxy.Tests/Proxy/TagValueCaptureTests.cs +++ b/mbproxy/tests/Mbproxy.Tests/Proxy/TagValueCaptureTests.cs @@ -174,4 +174,38 @@ public sealed class TagValueCaptureTests await Task.WhenAll([.. writers, reader]); tornObserved.ShouldBeFalse("Snapshot must never observe a torn (half-updated) slot"); } + + [Fact] + public async Task ConcurrentRecordAndDisarm_LeavesNoStaleObservation() + { + // M7 regression: Record() checks _armed then writes; Disarm() flips _armed then + // clears the slots. A Record that passes the check while armed, then has Disarm + // run, then writes, would strand a stale observation on a disarmed capture — + // breaking the "reopened page shows no stale data" contract. Record's re-check + // after the write must undo that. The capture ends disarmed (the toggler's last + // op is Disarm), so a clean Snapshot is a deterministic post-condition of the fix. + var capture = Make((100, 16)); + var ct = TestContext.Current.CancellationToken; + + var recorder = Task.Run(() => + { + for (int i = 0; i < 400_000; i++) + capture.Record(100, 0x1234, 0, 1234, CaptureDirection.Read); + }, ct); + + var toggler = Task.Run(() => + { + for (int i = 0; i < 80_000; i++) + { + capture.Arm(); + capture.Disarm(); + } + }, ct); + + await Task.WhenAll(recorder, toggler); + + capture.IsArmed.ShouldBeFalse(); + capture.Snapshot().ShouldAllBe(s => s.UpdatedAtUtc == null, + "a disarmed capture must never retain a recorded observation"); + } }