diff --git a/mbproxy/src/Mbproxy/Admin/StatusDto.cs b/mbproxy/src/Mbproxy/Admin/StatusDto.cs index 7e878a8..ec44d91 100644 --- a/mbproxy/src/Mbproxy/Admin/StatusDto.cs +++ b/mbproxy/src/Mbproxy/Admin/StatusDto.cs @@ -58,7 +58,13 @@ public sealed record PlcPdusStatus( long Forwarded, FcCounts ByFc, long RewrittenSlots, - long PartialBcdWarnings); + long PartialBcdWarnings, + /// + /// Phase 12 (W2.22) — count of BCD-rewriter slot decisions where the wire value was + /// not a valid BCD nibble pattern (e.g. 0xABCD at a tag address). The slot + /// passes through unrewritten and this counter increments. + /// + long InvalidBcdWarnings); /// Per-function-code request counts. public sealed record FcCounts( @@ -103,7 +109,12 @@ public sealed record ExceptionCounts( long Code01, long Code02, long Code03, - long Code04); + long Code04, + /// + /// Phase 12 (W2.22) — backend exceptions whose response code is not 01–04 (e.g. 0x06 + /// Server Device Busy, 0x0B Gateway Target Failed To Respond, vendor-specific codes). + /// + long CodeOther); /// Byte-transfer counters. public sealed record PlcBytesStatus( diff --git a/mbproxy/src/Mbproxy/Admin/StatusHtmlRenderer.cs b/mbproxy/src/Mbproxy/Admin/StatusHtmlRenderer.cs index 5c39331..9d58c0c 100644 --- a/mbproxy/src/Mbproxy/Admin/StatusHtmlRenderer.cs +++ b/mbproxy/src/Mbproxy/Admin/StatusHtmlRenderer.cs @@ -75,7 +75,7 @@ internal static class StatusHtmlRenderer sb.Append("NameHostPortState"); sb.Append("ClientsPDUs fwdFC03FC04"); sb.Append("FC06FC16FC?BCD slots"); - sb.Append("Partial BCDEx 01Ex 02Ex 03Ex 04"); + sb.Append("Partial BCDInvalid BCDEx 01Ex 02Ex 03Ex 04Ex ?"); sb.Append("RTT msBytes inBytes out"); // Phase 9: multiplexer telemetry columns. sb.Append("In-flightMax in-flightTxId wraps"); @@ -141,10 +141,12 @@ internal static class StatusHtmlRenderer sb.Append("").Append(plc.Pdus.ByFc.Other).Append(""); sb.Append("").Append(plc.Pdus.RewrittenSlots).Append(""); sb.Append("").Append(plc.Pdus.PartialBcdWarnings).Append(""); + sb.Append("").Append(plc.Pdus.InvalidBcdWarnings).Append(""); sb.Append("").Append(plc.Backend.ExceptionsByCode.Code01).Append(""); sb.Append("").Append(plc.Backend.ExceptionsByCode.Code02).Append(""); sb.Append("").Append(plc.Backend.ExceptionsByCode.Code03).Append(""); sb.Append("").Append(plc.Backend.ExceptionsByCode.Code04).Append(""); + sb.Append("").Append(plc.Backend.ExceptionsByCode.CodeOther).Append(""); sb.Append("").Append(plc.Backend.LastRoundTripMs.ToString("F1")).Append(""); sb.Append("").Append(plc.Bytes.UpstreamIn).Append(""); sb.Append("").Append(plc.Bytes.UpstreamOut).Append(""); diff --git a/mbproxy/src/Mbproxy/Admin/StatusSnapshotBuilder.cs b/mbproxy/src/Mbproxy/Admin/StatusSnapshotBuilder.cs index ee4ffbd..6a2ad5e 100644 --- a/mbproxy/src/Mbproxy/Admin/StatusSnapshotBuilder.cs +++ b/mbproxy/src/Mbproxy/Admin/StatusSnapshotBuilder.cs @@ -129,7 +129,8 @@ internal sealed class StatusSnapshotBuilder Forwarded: counters.PdusForwarded, ByFc: new FcCounts(counters.Fc03, counters.Fc04, counters.Fc06, counters.Fc16, counters.FcOther), RewrittenSlots: counters.RewrittenSlots, - PartialBcdWarnings: counters.PartialBcdWarnings), + PartialBcdWarnings: counters.PartialBcdWarnings, + InvalidBcdWarnings: counters.InvalidBcdWarnings), Backend: new PlcBackendStatus( ConnectsSuccess: connectsSuccess, ConnectsFailed: connectsFailed, @@ -137,7 +138,8 @@ internal sealed class StatusSnapshotBuilder counters.BackendException01, counters.BackendException02, counters.BackendException03, - counters.BackendException04), + counters.BackendException04, + counters.BackendExceptionOther), LastRoundTripMs: counters.LastRoundTripMs, InFlight: counters.InFlightCount, MaxInFlight: counters.MaxInFlight, diff --git a/mbproxy/src/Mbproxy/Bcd/BcdTagMapBuilder.cs b/mbproxy/src/Mbproxy/Bcd/BcdTagMapBuilder.cs index f673ac8..c0d4971 100644 --- a/mbproxy/src/Mbproxy/Bcd/BcdTagMapBuilder.cs +++ b/mbproxy/src/Mbproxy/Bcd/BcdTagMapBuilder.cs @@ -53,6 +53,32 @@ public static class BcdTagMapBuilder var errors = new List(); var warnings = new List(); + // Phase 12 (W2.11) — duplicate-address detection happens BEFORE the working + // dictionary collapses keys. Iterating each list independently catches duplicates + // that would otherwise be silently last-write-wins'd by the dictionary indexer. + // Cross-list collisions (same address in BOTH Global and Add) are the documented + // width-override pattern and must NOT be flagged — only intra-list duplicates fail. + // Without this fix the DuplicateAddress validation error was dead code (the + // post-collapse dict has unique keys by construction). + static void DetectIntraListDuplicates( + IEnumerable source, string sourceName, List errors) + { + var seen = new HashSet(); + foreach (var tag in source) + { + if (!seen.Add(tag.Address)) + { + errors.Add(new BcdError(BcdValidationError.DuplicateAddress, + $"Address {tag.Address} appears more than once in {sourceName}.", + tag.Address)); + } + } + } + + DetectIntraListDuplicates(global.Global, "Global", errors); + if (perPlc?.Add is { } addListForDup) + DetectIntraListDuplicates(addListForDup, "PerPlc.Add", errors); + // ── Step 1: collect the working set keyed by address ───────────────── // Dictionary preserves last-write-wins semantics for the Add override. var working = new Dictionary(global.Global.Count); @@ -82,7 +108,6 @@ public static class BcdTagMapBuilder // ── Step 4: validate the resolved list ─────────────────────────────── // We build a validated-entries list; only clean entries go into the map. var validated = new Dictionary(working.Count); - var seenAddresses = new HashSet(working.Count); foreach (var (addr, opt) in working) { @@ -94,14 +119,6 @@ public static class BcdTagMapBuilder continue; } - // Duplicate address check. - if (!seenAddresses.Add(addr)) - { - errors.Add(new BcdError(BcdValidationError.DuplicateAddress, - $"Address {addr} appears more than once in the resolved tag list.", addr)); - continue; - } - // Phase 11 — resolve the effective per-tag cache TTL: // explicit per-tag (including 0) wins; otherwise fall back to per-PLC default. int resolvedTtl = opt.CacheTtlMs ?? perPlcDefaultCacheTtlMs; @@ -111,6 +128,10 @@ public static class BcdTagMapBuilder } // High-register collision check (only meaningful for 32-bit entries). + // Phase 12 (W2.12) — dedupe symmetric reports. Two 32-bit tags whose pairs collide + // (e.g. (100,W=32) and (101,W=32)) would otherwise produce two BcdErrors — one + // from each direction. Track reported (low,high) pairs so each collision logs once. + var reportedCollisions = new HashSet<(ushort, ushort)>(); foreach (var tag in validated.Values) { if (!tag.IsThirtyTwoBit) @@ -119,6 +140,13 @@ public static class BcdTagMapBuilder ushort highReg = tag.HighRegister; if (validated.TryGetValue(highReg, out var collision)) { + // Canonicalise the pair so (a,b) and (b,a) collapse. + var pair = tag.Address < collision.Address + ? (tag.Address, collision.Address) + : (collision.Address, tag.Address); + if (!reportedCollisions.Add(pair)) + continue; + errors.Add(new BcdError(BcdValidationError.OverlappingHighRegister, $"32-bit BCD tag at address {tag.Address} has its high register " + $"({highReg}) colliding with the entry at address {collision.Address}.", diff --git a/mbproxy/src/Mbproxy/Configuration/ConfigReconciler.cs b/mbproxy/src/Mbproxy/Configuration/ConfigReconciler.cs index 116a703..dbaadc6 100644 --- a/mbproxy/src/Mbproxy/Configuration/ConfigReconciler.cs +++ b/mbproxy/src/Mbproxy/Configuration/ConfigReconciler.cs @@ -1,3 +1,4 @@ +using System.Collections.Concurrent; using System.Threading.Channels; using Mbproxy.Bcd; using Mbproxy.Options; @@ -47,10 +48,19 @@ internal sealed partial class ConfigReconciler : IDisposable private readonly ServiceCounters _serviceCounters; // The supervisor dictionary is set by ProxyWorker after initial startup. - // All mutations happen inside ApplyAsync which is serialised by the semaphore. - private Dictionary? _supervisors; + // Phase 12 (W2.3) — ConcurrentDictionary so the per-PLC Add/Remove/Restart task + // continuations inside ApplyUnderLockAsync can mutate it concurrently. The outer Apply + // is serialised by _applySemaphore but the inner Task.WhenAll runs in parallel. + private ConcurrentDictionary? _supervisors; private MbproxyOptions? _currentOptions; + // Phase 12 (W2.1) — live accessor for ReadCoalescingOptions, threaded through Attach + // so PLCs added or restarted via hot-reload honour the current + // `Mbproxy.Resilience.ReadCoalescing.{Enabled,MaxParties}` values. Without this, + // reconciler-built supervisors silently used the default `new ReadCoalescingOptions()`, + // and a hot-reload of `Enabled = false` did not propagate to those supervisors. + private Func? _coalescingAccessor; + // ── Debounce + serialisation machinery ─────────────────────────────────────────────── // Channel carries Unit to signal "something changed — please check". @@ -100,18 +110,22 @@ internal sealed partial class ConfigReconciler : IDisposable // ── Wire-up called by ProxyWorker after initial startup ────────────────────────────── /// - /// Provides the reconciler with the supervisor dictionary and the initial options - /// snapshot. Must be called exactly once by before - /// any OnChange events can arrive (i.e. immediately after the supervisors are - /// created). Thread-safe: the reconciler hasn't started processing changes yet at this - /// point. + /// Provides the reconciler with the supervisor dictionary, the initial options snapshot, + /// and (Phase 12 W2.1) the live accessor that + /// add/restart supervisors must use so a hot-reloaded + /// Mbproxy.Resilience.ReadCoalescing.Enabled propagates to them. Must be called + /// exactly once by before any OnChange events + /// can arrive (i.e. immediately after the supervisors are created). Thread-safe: the + /// reconciler hasn't started processing changes yet at this point. /// public void Attach( - Dictionary supervisors, - MbproxyOptions initialOptions) + ConcurrentDictionary supervisors, + MbproxyOptions initialOptions, + Func? coalescingAccessor = null) { - _supervisors = supervisors; - _currentOptions = initialOptions; + _supervisors = supervisors; + _currentOptions = initialOptions; + _coalescingAccessor = coalescingAccessor; } // ── ApplyAsync (exposed for tests) ─────────────────────────────────────────────────── @@ -229,13 +243,12 @@ internal sealed partial class ConfigReconciler : IDisposable if (plan.ToRemove.Count > 0) { var removeTasks = plan.ToRemove - .Where(name => _supervisors.ContainsKey(name)) .Select(async name => { + if (!_supervisors.TryRemove(name, out var s)) + return; try { - var s = _supervisors[name]; - _supervisors.Remove(name); using var stopCts = CancellationTokenSource.CreateLinkedTokenSource(ct); stopCts.CancelAfter(TimeSpan.FromSeconds(10)); await s.StopAsync(stopCts.Token).ConfigureAwait(false); @@ -266,9 +279,8 @@ internal sealed partial class ConfigReconciler : IDisposable try { // Stop old supervisor. - if (_supervisors.TryGetValue(name, out var old)) + if (_supervisors.TryRemove(name, out var old)) { - _supervisors.Remove(name); using var stopCts = CancellationTokenSource.CreateLinkedTokenSource(ct); stopCts.CancelAfter(TimeSpan.FromSeconds(10)); await old.StopAsync(stopCts.Token).ConfigureAwait(false); @@ -301,7 +313,8 @@ internal sealed partial class ConfigReconciler : IDisposable newCtx, recoveryPipeline, _loggerFactory.CreateLogger(), - backendPipeline); + backendPipeline, + _coalescingAccessor); _supervisors[name] = newSupervisor; await newSupervisor.StartAsync(ct).ConfigureAwait(false); @@ -385,7 +398,8 @@ internal sealed partial class ConfigReconciler : IDisposable newCtx, recoveryPipeline, _loggerFactory.CreateLogger(), - backendPipeline); + backendPipeline, + _coalescingAccessor); _supervisors[plcNew.Name] = newSupervisor; await newSupervisor.StartAsync(ct).ConfigureAwait(false); diff --git a/mbproxy/src/Mbproxy/Configuration/ReloadValidator.cs b/mbproxy/src/Mbproxy/Configuration/ReloadValidator.cs index f2a4781..7ff9e4d 100644 --- a/mbproxy/src/Mbproxy/Configuration/ReloadValidator.cs +++ b/mbproxy/src/Mbproxy/Configuration/ReloadValidator.cs @@ -75,11 +75,27 @@ internal static class ReloadValidator // ── 4. Per-PLC tag-map build ────────────────────────────────────────── // BcdTagMapBuilder.Build is the single source of truth for tag-list // well-formedness; we must not duplicate its validation logic here. + // Phase 12 (W2.10) — also re-check the RESOLVED per-tag CacheTtlMs against + // AllowLongTtl. The raw-input check at section 5 covers explicit per-tag and + // per-PLC-default values, but defensively re-validating the post-fold values + // catches any future fold logic that produces a value above the gate. + bool allowLongTtlForResolved = next.Cache.AllowLongTtl; foreach (var plc in next.Plcs) { var result = BcdTagMapBuilder.Build(next.BcdTags, plc.BcdTags, plc.DefaultCacheTtlMs); foreach (var err in result.Errors) errs.Add($"Plc '{plc.Name}': BCD tag map error ({err.Kind}): {err.Message}"); + + if (!allowLongTtlForResolved) + { + foreach (var tag in result.Map.All) + { + if (tag.CacheTtlMs > 60_000) + errs.Add( + $"Plc '{plc.Name}': resolved CacheTtlMs for Address {tag.Address} is " + + $"{tag.CacheTtlMs} ms (exceeds 60_000) without Cache.AllowLongTtl=true."); + } + } } // ── 5. Cache TTL bounds (Phase 11) ──────────────────────────────────── @@ -113,6 +129,18 @@ internal static class ReloadValidator if (next.Cache.EvictionIntervalMs < 0) errs.Add($"Cache.EvictionIntervalMs must be >= 0; got {next.Cache.EvictionIntervalMs}."); + // Phase 12 (W2.18) — Connection timeouts must be > 0. A reload that sets any + // of these to 0 or negative would break the runtime; reject the reload as a whole. + if (next.Connection.BackendConnectTimeoutMs <= 0) + errs.Add( + $"Connection.BackendConnectTimeoutMs must be > 0; got {next.Connection.BackendConnectTimeoutMs}."); + if (next.Connection.BackendRequestTimeoutMs <= 0) + errs.Add( + $"Connection.BackendRequestTimeoutMs must be > 0; got {next.Connection.BackendRequestTimeoutMs}."); + if (next.Connection.GracefulShutdownTimeoutMs <= 0) + errs.Add( + $"Connection.GracefulShutdownTimeoutMs must be > 0; got {next.Connection.GracefulShutdownTimeoutMs}."); + errors = errs; return errs.Count == 0; } diff --git a/mbproxy/src/Mbproxy/Diagnostics/EventLogBridge.cs b/mbproxy/src/Mbproxy/Diagnostics/EventLogBridge.cs index 74216b1..a2b5f4f 100644 --- a/mbproxy/src/Mbproxy/Diagnostics/EventLogBridge.cs +++ b/mbproxy/src/Mbproxy/Diagnostics/EventLogBridge.cs @@ -29,10 +29,19 @@ internal sealed class EventLogBridge : ILogEventSink private const int MaxMessageBytes = 32 * 1024; // 32 KB Event Log limit private readonly bool _enabled; + // Phase 12 (W2.23) — cache the source-exists check at construction so Emit doesn't + // hit the registry on every Error+ log line. A missing source after start requires a + // service restart to pick up; in practice install.ps1 registers it once at install. + private readonly bool _sourceExists; public EventLogBridge(bool enabled) { _enabled = enabled; + if (_enabled) + { + try { _sourceExists = EventLog.SourceExists(Source); } + catch { _sourceExists = false; } + } } /// @@ -41,9 +50,9 @@ internal sealed class EventLogBridge : ILogEventSink if (!_enabled) return; if (logEvent.Level < LogEventLevel.Error) return; - // Check that the source exists; if not, silently swallow — the service - // account may not be able to create it and we must not crash the logger. - if (!EventLog.SourceExists(Source)) return; + // Cached at construction (W2.23) — silently swallow if the source isn't registered. + // The service account may not be able to create it and we must not crash the logger. + if (!_sourceExists) return; string message = logEvent.RenderMessage(); diff --git a/mbproxy/src/Mbproxy/Mbproxy.csproj b/mbproxy/src/Mbproxy/Mbproxy.csproj index 3bf21be..ce110a0 100644 --- a/mbproxy/src/Mbproxy/Mbproxy.csproj +++ b/mbproxy/src/Mbproxy/Mbproxy.csproj @@ -49,7 +49,15 @@ - + + + PreserveNewest diff --git a/mbproxy/src/Mbproxy/Options/MbproxyOptions.cs b/mbproxy/src/Mbproxy/Options/MbproxyOptions.cs index 0fb98aa..66215c6 100644 --- a/mbproxy/src/Mbproxy/Options/MbproxyOptions.cs +++ b/mbproxy/src/Mbproxy/Options/MbproxyOptions.cs @@ -94,6 +94,19 @@ public sealed class MbproxyOptionsValidator : IValidateOptions if (options.Cache.EvictionIntervalMs < 0) errors.Add($"Cache.EvictionIntervalMs must be >= 0; got {options.Cache.EvictionIntervalMs}."); + // Phase 12 (W2.18) — Connection timeouts must be strictly positive. A 0 or negative + // value produces a CancelAfter(0) that fires immediately and breaks every backend + // connect/request. + if (options.Connection.BackendConnectTimeoutMs <= 0) + errors.Add( + $"Connection.BackendConnectTimeoutMs must be > 0; got {options.Connection.BackendConnectTimeoutMs}."); + if (options.Connection.BackendRequestTimeoutMs <= 0) + errors.Add( + $"Connection.BackendRequestTimeoutMs must be > 0; got {options.Connection.BackendRequestTimeoutMs}."); + if (options.Connection.GracefulShutdownTimeoutMs <= 0) + errors.Add( + $"Connection.GracefulShutdownTimeoutMs must be > 0; got {options.Connection.GracefulShutdownTimeoutMs}."); + return errors.Count > 0 ? ValidateOptionsResult.Fail(errors) : ValidateOptionsResult.Success; diff --git a/mbproxy/src/Mbproxy/Proxy/BcdPduPipeline.cs b/mbproxy/src/Mbproxy/Proxy/BcdPduPipeline.cs index 581e8ad..1c2448d 100644 --- a/mbproxy/src/Mbproxy/Proxy/BcdPduPipeline.cs +++ b/mbproxy/src/Mbproxy/Proxy/BcdPduPipeline.cs @@ -156,7 +156,15 @@ internal sealed class BcdPduPipeline : IPduPipeline ushort startAddress = (ushort)((pdu[1] << 8) | pdu[2]); ushort qty = (ushort)((pdu[3] << 8) | pdu[4]); - // byte byteCount = pdu[5]; (qty * 2, not used directly) + + // Phase 12 (W2.14) — validate the request is fully sized for `qty` registers + // (each 2 bytes after the byteCount byte). A client claiming qty=10 with only + // 4 bytes of register data would otherwise have its BCD slots silently skipped + // by the per-slot bounds check below — half the request rewritten, half not. + // Returning here passes the malformed PDU through unchanged so the PLC's own + // validator surfaces the protocol error. + if (pdu.Length < 6 + qty * 2) + return; if (!ctx.TagMap.TryGetForRange(startAddress, qty, out var hits)) return; // no BCD tags in this range @@ -202,6 +210,22 @@ internal sealed class BcdPduPipeline : IPduPipeline ushort clientLow = (ushort)((pdu[lowByteOff] << 8) | pdu[lowByteOff + 1]); ushort clientHigh = (ushort)((pdu[highByteOff] << 8) | pdu[highByteOff + 1]); + // Phase 12 (W2.13) — validate that BOTH input words are within the + // base-10000-digit range BEFORE reconstructing. Without this guard, a + // client writing (high=9999, low=9999) silently mutates to (high=9998, + // low=9999) because `9999 * 10_000 + 9999 = 99_989_999` is still <= the + // 32-bit BCD ceiling, so Encode32 accepts it and rewrites — losing 1 from + // the high word. The unconventional wire format ("two base-10000 CDAB + // digits", per design.md:123) means each word independently must be + // 0..9999 to round-trip cleanly. + if (clientLow > 9999 || clientHigh > 9999) + { + RewriterLogEvents.InvalidBcd(ctx.Logger, ctx.PlcName, tag.Address, + clientLow, "Write"); + ctx.Counters.IncrementInvalidBcd(); + continue; + } + // Reconstruct the 32-bit binary value (CDAB: low-word = low digits). int binaryValue = clientHigh * 10_000 + clientLow; diff --git a/mbproxy/src/Mbproxy/Proxy/Multiplexing/PlcMultiplexer.cs b/mbproxy/src/Mbproxy/Proxy/Multiplexing/PlcMultiplexer.cs index c306565..7e18a7c 100644 --- a/mbproxy/src/Mbproxy/Proxy/Multiplexing/PlcMultiplexer.cs +++ b/mbproxy/src/Mbproxy/Proxy/Multiplexing/PlcMultiplexer.cs @@ -88,7 +88,11 @@ internal sealed class PlcMultiplexer : IAsyncDisposable, IMultiplexCountersProvi private Task? _backendReaderTask; private readonly CancellationTokenSource _disposeCts = new(); - private bool _disposed; + // Phase 12 (W2.2) — volatile so the disposing thread's write is observed by every + // hot-path reader (OnUpstreamFrameAsync, ReplaceContext, Attach, etc.) without a + // separate fence. On x86/x64 plain reads happen to give acquire-release semantics, so + // this is defense for ARM hosts and future portability. + private volatile bool _disposed; private Task? _watchdogTask; public PlcMultiplexer( @@ -240,7 +244,11 @@ internal sealed class PlcMultiplexer : IAsyncDisposable, IMultiplexCountersProvi } _pipes.Clear(); - _disposeCts.Dispose(); + // Phase 12 (W2.5, W2.6) — guard the CTS dispose against a watchdog tick that + // raced past the WaitAsync above (e.g. a slow Task.Delay completion observing + // cancellation late). Also dispose the connect-gate semaphore. + try { _disposeCts.Dispose(); } catch (ObjectDisposedException) { /* already disposed */ } + try { _connectGate.Dispose(); } catch (ObjectDisposedException) { /* already disposed */ } } // ── Backend connect / teardown ──────────────────────────────────────────── @@ -522,9 +530,14 @@ internal sealed class PlcMultiplexer : IAsyncDisposable, IMultiplexCountersProvi // cache-eligible (resolvedTtlMs > 0). // * FC06/FC16 successful responses invalidate every cached entry whose // address range overlaps the write. + // + // Phase 12 (W2.7) — exception bit comes from the post-rewriter buffer + // (the rewriter never touches the FC byte today, but reading from + // inFlight.Fc would lose the exception bit). The base FC for routing + // decisions uses inFlight.Fc — the request side knows what was sent. if (_ctx.Cache is { } postCache) { - byte fcInResponse = frame[MbapFrame.HeaderSize]; // post-rewriter, but the FC byte is never rewritten + byte fcInResponse = frame[MbapFrame.HeaderSize]; bool isException = (fcInResponse & 0x80) != 0; if (!isException) @@ -555,6 +568,16 @@ internal sealed class PlcMultiplexer : IAsyncDisposable, IMultiplexCountersProvi } else if (inFlight.Fc is 0x06 or 0x10) { + // Phase 12 (W2.9) — the design contract "invalidations during a + // recovering listener state are skipped" (design.md:203) is + // upheld IMPLICITLY here: invalidation only fires inside the + // backend reader task when a non-exception FC06/FC16 response + // arrives. A `Recovering` listener has no backend reader (the + // multiplexer is torn down between recovery attempts), so no + // response can land here, so no invalidation. The gating is + // structural, not conditional. If a future change ever produces + // a write response off the live backend, an explicit recovering- + // state check would need to be added. int invalidated = postCache.Invalidate( inFlight.UnitId, inFlight.StartAddress, inFlight.Qty); if (invalidated > 0) @@ -692,6 +715,12 @@ internal sealed class PlcMultiplexer : IAsyncDisposable, IMultiplexCountersProvi return; } + // Per design contract: "miss" = "fell through to coalescing/backend". + // When two upstream peers issue the same cache-eligible read, both increment + // CacheMiss; only one then opens a backend round-trip (the second coalesces + // onto the first via the InFlightByKey path below). So `CacheMiss` does NOT + // equal "produced a backend round-trip" — it equals "did not find a fresh + // cache entry". The identity `Hit + Miss = cache-eligible requests` holds. _ctx.Counters.IncrementCacheMiss(); CacheLogEvents.Miss(_logger, _plc.Name, unitId, fcByte, startAddr, qty); } @@ -786,7 +815,11 @@ internal sealed class PlcMultiplexer : IAsyncDisposable, IMultiplexCountersProvi return; } - // Coalesce miss: we just opened a fresh in-flight entry. + // Coalesce miss: this request did not attach to an in-flight peer. Per the + // design contract `coalescedHitCount + coalescedMissCount = total FC03/FC04`, + // so even saturation-failure paths (factory below returns null inFlightForSend) + // count as a miss — every FC03/FC04 entered the coalescing path exactly once. + // "Miss" here means "did not coalesce", NOT "produced a backend round-trip". _ctx.Counters.IncrementCoalescedMiss(); CoalescingLogEvents.Miss(_logger, _plc.Name, unitId, fcByte, startAddr, qty); diff --git a/mbproxy/src/Mbproxy/Proxy/Multiplexing/UpstreamPipe.cs b/mbproxy/src/Mbproxy/Proxy/Multiplexing/UpstreamPipe.cs index 8a77ddb..e1f58e2 100644 --- a/mbproxy/src/Mbproxy/Proxy/Multiplexing/UpstreamPipe.cs +++ b/mbproxy/src/Mbproxy/Proxy/Multiplexing/UpstreamPipe.cs @@ -49,7 +49,9 @@ internal sealed partial class UpstreamPipe : IAsyncDisposable // Internal CTS lets the multiplexer signal "drop this pipe now" without waiting for // the upstream socket to close cleanly. private readonly CancellationTokenSource _cts = new(); - private bool _disposed; + // Phase 12 (W2.2) — volatile so writes from DisposeAsync are observed by IsAlive / + // TrySendResponse on other threads without a fence. + private volatile bool _disposed; // Phase 9: per-pipe forwarded-PDU counter (replaces the per-pair counter from the // 1:1 model). Read by the status page. diff --git a/mbproxy/src/Mbproxy/Proxy/ProxyWorker.cs b/mbproxy/src/Mbproxy/Proxy/ProxyWorker.cs index 96468d9..9bab154 100644 --- a/mbproxy/src/Mbproxy/Proxy/ProxyWorker.cs +++ b/mbproxy/src/Mbproxy/Proxy/ProxyWorker.cs @@ -1,3 +1,4 @@ +using System.Collections.Concurrent; using System.Diagnostics; using Mbproxy.Admin; using Mbproxy.Bcd; @@ -49,7 +50,13 @@ internal sealed partial class ProxyWorker : BackgroundService // Phase 06: supervisors are now managed jointly by ProxyWorker (initial bootstrap) // and ConfigReconciler (subsequent hot-reload changes). The dictionary is shared // via ConfigReconciler.Attach() after initial startup. - private readonly Dictionary _supervisors = new(StringComparer.Ordinal); + // + // Phase 12 (W2.3) — ConcurrentDictionary because ConfigReconciler mutates this from + // parallel Task.WhenAll continuations (Add/Remove/Restart paths). The outer Apply is + // serialised by a semaphore but the inner per-PLC tasks run concurrently. Status-page + // reads via IReadOnlyDictionary still work without locking. + private readonly ConcurrentDictionary _supervisors = + new(StringComparer.Ordinal); /// /// Read-only view of the live supervisor dictionary. Consumed by Phase 07's @@ -164,7 +171,11 @@ internal sealed partial class ProxyWorker : BackgroundService // initial options snapshot. The reconciler won't process OnChange events until // after this call — the brief window between Attach and first supervisor start // is safe because the channel signal only enqueues; apply runs asynchronously. - _reconciler.Attach(_supervisors, opts); + // Phase 12 (W2.1) — also pass the live coalescing accessor so reconciler-built + // supervisors (add/restart paths) honour hot-reloaded ReadCoalescing values. + Func reconcilerCoalescingAccessor = + () => _options.CurrentValue.Resilience.ReadCoalescing; + _reconciler.Attach(_supervisors, opts, reconcilerCoalescingAccessor); if (_supervisors.Count == 0) { @@ -252,9 +263,13 @@ internal sealed partial class ProxyWorker : BackgroundService await base.StopAsync(cancellationToken).ConfigureAwait(false); var sw = Stopwatch.StartNew(); + // Phase 12 (W2.20) — supervisor stop deadline read from the live config so a + // hot-reloaded GracefulShutdownTimeoutMs is honoured. Previously hard-coded 5 s. + // The supervisor stop budget is bounded by the same total-shutdown budget. + int gracefulMs = _options.CurrentValue.Connection.GracefulShutdownTimeoutMs; // ── 1. Stop accepting new connections ───────────────────────────────────────── - using var stopCts = new CancellationTokenSource(TimeSpan.FromSeconds(5)); + using var stopCts = new CancellationTokenSource(TimeSpan.FromMilliseconds(gracefulMs)); using var linked = CancellationTokenSource.CreateLinkedTokenSource( stopCts.Token, cancellationToken); @@ -272,9 +287,8 @@ internal sealed partial class ProxyWorker : BackgroundService } // ── 2. Drain in-flight PDUs ─────────────────────────────────────────────────── - // Reads the current configured deadline so a hot-reloaded - // GracefulShutdownTimeoutMs is honoured at stop time, not frozen at process start. - int drainDeadlineMs = _options.CurrentValue.Connection.GracefulShutdownTimeoutMs; + // Same `gracefulMs` budget the supervisor-stop step used. + int drainDeadlineMs = gracefulMs; int inFlightAtCancel = 0; if (drainDeadlineMs > 0) diff --git a/mbproxy/src/Mbproxy/Proxy/Supervision/PlcListenerSupervisor.cs b/mbproxy/src/Mbproxy/Proxy/Supervision/PlcListenerSupervisor.cs index d3f986d..f19ca7d 100644 --- a/mbproxy/src/Mbproxy/Proxy/Supervision/PlcListenerSupervisor.cs +++ b/mbproxy/src/Mbproxy/Proxy/Supervision/PlcListenerSupervisor.cs @@ -1,4 +1,5 @@ using Mbproxy.Options; +using Mbproxy.Proxy.Cache; using Mbproxy.Proxy.Multiplexing; using Polly; @@ -66,6 +67,13 @@ internal sealed partial class PlcListenerSupervisor : IAsyncDisposable private bool _disposed; + // Phase 12 (W2.15) — completes when the supervisor has transitioned out of Stopped + // for the first time (reached Bound or Recovering). Replaces the previous busy-poll + // implementation in WaitForInitialBindAttemptAsync, which raced fast Stopped→Bound→ + // Stopped transitions and never exited if the supervisor task threw inside Polly. + private readonly TaskCompletionSource _firstAttemptCompleted = new( + TaskCreationOptions.RunContinuationsAsynchronously); + // ── Public surface ──────────────────────────────────────────────────────────────────── public string PlcName => _plc.Name; @@ -123,6 +131,16 @@ internal sealed partial class PlcListenerSupervisor : IAsyncDisposable /// public Task StartAsync(CancellationToken ct) { + // Phase 12 (W2.16) — refuse to re-Start an already-running or already-disposed + // supervisor. The original code reassigned _supervisorCts unconditionally, which + // leaked the previous CTS and could leave a zombie task running against an + // unobserved token. The supervisor's state machine has exactly one Start. + if (_disposed) + throw new ObjectDisposedException(nameof(PlcListenerSupervisor)); + if (_state != SupervisorState.Stopped || !_supervisorTask.IsCompleted) + throw new InvalidOperationException( + $"Supervisor for Plc='{_plc.Name}' has already been started."); + _supervisorCts = CancellationTokenSource.CreateLinkedTokenSource(ct); _supervisorTask = Task.Run(() => RunSupervisorAsync(_supervisorCts.Token), CancellationToken.None); return Task.CompletedTask; @@ -133,13 +151,22 @@ internal sealed partial class PlcListenerSupervisor : IAsyncDisposable /// (transitioned to or /// ). /// Returns immediately if the supervisor is already past that point. + /// + /// Phase 12 (W2.15) — backed by a set + /// when the supervisor task first transitions out of . + /// Replaces the previous 10 ms busy-poll which raced fast bind+stop sequences and could + /// hang if the supervisor task threw before any state write happened. /// public async Task WaitForInitialBindAttemptAsync(CancellationToken ct) { - while (_state == SupervisorState.Stopped && !ct.IsCancellationRequested - && !_supervisorTask.IsCompleted) + if (_firstAttemptCompleted.Task.IsCompleted) return; + try { - await Task.Delay(10, ct).ConfigureAwait(false); + await _firstAttemptCompleted.Task.WaitAsync(ct).ConfigureAwait(false); + } + catch (OperationCanceledException) + { + // Caller cancelled; not a fault — same observable behaviour as the prior poll. } } @@ -173,11 +200,43 @@ internal sealed partial class PlcListenerSupervisor : IAsyncDisposable } } - /// Returns a point-in-time snapshot of this supervisor's state. - public SupervisorSnapshot Snapshot() => new( - State: _state, - LastBindError: _lastBindError, - RecoveryAttempts: Interlocked.CompareExchange(ref _recoveryAttempts, 0, 0)); + /// + /// Returns a point-in-time snapshot of this supervisor's state. + /// + /// Phase 12 (W2.17) — reads the three observable fields under a single + /// lock so the status page can never report inconsistent triples like + /// (State=Bound, LastBindError=<previous>, RecoveryAttempts>0). The + /// supervisor task uses which takes the same lock, so a + /// snapshot reads a transition-consistent view. + /// + public SupervisorSnapshot Snapshot() + { + lock (_snapshotLock) + { + return new SupervisorSnapshot( + State: _state, + LastBindError: _lastBindError, + RecoveryAttempts: _recoveryAttempts); + } + } + + private readonly object _snapshotLock = new(); + + /// + /// Phase 12 (W2.17) — atomic three-field transition. State, lastBindError, and + /// (optionally) the recoveryAttempts increment all happen under one lock so a + /// concurrent never sees a half-applied transition. + /// + private void TransitionTo(SupervisorState newState, string? lastBindError, bool incrementRecoveryAttempt) + { + lock (_snapshotLock) + { + _state = newState; + _lastBindError = lastBindError; + if (incrementRecoveryAttempt) + _recoveryAttempts++; + } + } /// /// Atomically swaps the per-PLC context (tag map + optional response cache) on the @@ -210,12 +269,16 @@ internal sealed partial class PlcListenerSupervisor : IAsyncDisposable // inside the Polly loop will pick up newCtx through _currentContext above. _currentListener?.Multiplexer?.ReplaceContext(newCtx); - // Phase 12 (W1.1 + W2.8 prereq) — drop the outgoing cache AFTER the swap so the - // running multiplexer can no longer reach it. Dispose stops the eviction loop and - // releases the timer. (The cache.flushed log event is W2.8 work; this Wave-1 fix - // is the "no longer in use, safe to drop" piece.) + // Phase 12 (W1.1 + W2.8) — drop the outgoing cache AFTER the swap so the running + // multiplexer can no longer reach it. Clear() snapshots the entry count for the + // mbproxy.cache.flushed log event before disposing the cache (which stops the + // eviction loop and releases the timer). if (oldCache is not null && !ReferenceEquals(oldCache, newCtx.Cache)) + { + int dropped = oldCache.Clear(); + CacheLogEvents.Flushed(_logger, _plc.Name, "tag-list-reload", dropped); oldCache.Dispose(); + } return Task.CompletedTask; } @@ -268,11 +331,10 @@ internal sealed partial class PlcListenerSupervisor : IAsyncDisposable _currentListener = null; await listener.DisposeAsync().ConfigureAwait(false); - Interlocked.Increment(ref _recoveryAttempts); - string reason = bindEx.Message; - string truncated = reason.Length > 256 ? reason[..256] : reason; - _lastBindError = truncated; - _state = SupervisorState.Recovering; + string truncated = Truncate(bindEx.Message, 256); + TransitionTo(SupervisorState.Recovering, truncated, incrementRecoveryAttempt: true); + // Phase 12 (W2.15) — signal the first transition out of Stopped. + _firstAttemptCompleted.TrySetResult(); // Also update the per-PLC counters if available (Phase 07 reads these). _currentContext?.Counters.IncrementRecoveryAttempt(truncated); @@ -297,9 +359,10 @@ internal sealed partial class PlcListenerSupervisor : IAsyncDisposable } // Clear the last bind error on a successful bind. - _lastBindError = null; + TransitionTo(SupervisorState.Bound, lastBindError: null, incrementRecoveryAttempt: false); _currentContext?.Counters.ClearLastBindError(); - _state = SupervisorState.Bound; + // Phase 12 (W2.15) — signal the first transition out of Stopped. + _firstAttemptCompleted.TrySetResult(); // ── Run the accept loop ────────────────────────────────────────── // RunAsync returns when: (a) token is cancelled (normal shutdown), @@ -324,10 +387,12 @@ internal sealed partial class PlcListenerSupervisor : IAsyncDisposable _currentListener = null; await listener.DisposeAsync().ConfigureAwait(false); - Interlocked.Increment(ref _recoveryAttempts); - string truncated = runEx.Message.Length > 256 ? runEx.Message[..256] : runEx.Message; - _lastBindError = truncated; - _state = SupervisorState.Recovering; + string truncated = Truncate(runEx.Message, 256); + TransitionTo(SupervisorState.Recovering, truncated, incrementRecoveryAttempt: true); + // Phase 12 (W2.15) — also signal first-attempt-completed in case the + // very first listener.RunAsync faulted before the bind-success path + // signalled it. + _firstAttemptCompleted.TrySetResult(); // Also update the per-PLC counters if available. _currentContext?.Counters.IncrementRecoveryAttempt(truncated); @@ -346,10 +411,8 @@ internal sealed partial class PlcListenerSupervisor : IAsyncDisposable // Otherwise (listener closed without cancellation — e.g., OS event), // treat as a fault and re-enter recovery. - Interlocked.Increment(ref _recoveryAttempts); const string unexpectedEnd = "Listener accept loop ended unexpectedly"; - _lastBindError = unexpectedEnd; - _state = SupervisorState.Recovering; + TransitionTo(SupervisorState.Recovering, unexpectedEnd, incrementRecoveryAttempt: true); _currentContext?.Counters.IncrementRecoveryAttempt(unexpectedEnd); LogListenerEnded(_logger, _plc.Name, _plc.ListenPort); throw new InvalidOperationException(unexpectedEnd); @@ -369,11 +432,26 @@ internal sealed partial class PlcListenerSupervisor : IAsyncDisposable } finally { - _state = SupervisorState.Stopped; + // Snapshot consistency: state goes back to Stopped without changing the last + // bind error so operators can still see WHY the supervisor exited. + lock (_snapshotLock) + { + _state = SupervisorState.Stopped; + } _currentListener = null; + // Phase 12 (W2.15) — defensive: if RunSupervisorAsync exits before any bind + // attempt fired (e.g. construction-time fault), unblock any awaiting + // WaitForInitialBindAttemptAsync caller so it doesn't hang. + _firstAttemptCompleted.TrySetResult(); } } + /// + /// Phase 12 (W2 cleanup) — single helper for the truncate-exception-message pattern + /// previously copy-pasted across three call sites. + /// + private static string Truncate(string s, int max) => s.Length > max ? s[..max] : s; + // ── IAsyncDisposable ───────────────────────────────────────────────────────────────── public async ValueTask DisposeAsync() diff --git a/mbproxy/src/Mbproxy/appsettings.json b/mbproxy/src/Mbproxy/appsettings.json deleted file mode 100644 index 24b80d2..0000000 --- a/mbproxy/src/Mbproxy/appsettings.json +++ /dev/null @@ -1,54 +0,0 @@ -{ - "Mbproxy": { - "BcdTags": { - "Global": [] - }, - "Plcs": [], - "AdminPort": 8080, - "Connection": { - "BackendConnectTimeoutMs": 3000, - "BackendRequestTimeoutMs": 3000 - }, - "Resilience": { - "BackendConnect": { - "MaxAttempts": 3, - "BackoffMs": [ 100, 500, 2000 ] - }, - "ListenerRecovery": { - "InitialBackoffMs": [ 1000, 2000, 5000, 15000, 30000 ], - "SteadyStateMs": 30000 - }, - "ReadCoalescing": { - "Enabled": true, - "MaxParties": 32 - } - } - }, - "Serilog": { - "Using": [ "Serilog.Sinks.Console", "Serilog.Sinks.File" ], - "MinimumLevel": { - "Default": "Information", - "Override": { - "Microsoft": "Warning", - "System": "Warning" - } - }, - "WriteTo": [ - { - "Name": "Console", - "Args": { - "outputTemplate": "[{Timestamp:HH:mm:ss} {Level:u3}] {Message:lj} {Properties:j}{NewLine}{Exception}" - } - }, - { - "Name": "File", - "Args": { - "path": "C:\\ProgramData\\mbproxy\\logs\\mbproxy-.log", - "rollingInterval": "Day", - "retainedFileCountLimit": 30, - "outputTemplate": "[{Timestamp:yyyy-MM-dd HH:mm:ss.fff zzz} {Level:u3}] {Message:lj} {Properties:j}{NewLine}{Exception}" - } - } - ] - } -} diff --git a/mbproxy/tests/Mbproxy.Tests/Admin/StatusHtmlRendererTests.cs b/mbproxy/tests/Mbproxy.Tests/Admin/StatusHtmlRendererTests.cs index 4a251e7..0afcdc1 100644 --- a/mbproxy/tests/Mbproxy.Tests/Admin/StatusHtmlRendererTests.cs +++ b/mbproxy/tests/Mbproxy.Tests/Admin/StatusHtmlRendererTests.cs @@ -43,10 +43,10 @@ public sealed class StatusHtmlRendererTests ListenPort: 5020, Listener: new PlcListenerStatus(state, lastBindError, recoveryAttempts), Clients: new PlcClientsStatus(clients?.Count ?? 0, clients ?? noClients), - Pdus: new PlcPdusStatus(100, new FcCounts(50, 10, 20, 15, 5), 30, 2), + Pdus: new PlcPdusStatus(100, new FcCounts(50, 10, 20, 15, 5), 30, 2, 0), Backend: new PlcBackendStatus( ConnectsSuccess: 0, ConnectsFailed: 0, - ExceptionsByCode: new ExceptionCounts(1, 0, 0, 0), + ExceptionsByCode: new ExceptionCounts(1, 0, 0, 0, 0), LastRoundTripMs: 3.5, InFlight: 0, MaxInFlight: 0, TxIdWraps: 0, DisconnectCascades: 0, QueueDepth: 0, diff --git a/mbproxy/tests/Mbproxy.Tests/Bcd/BcdTagMapBuilderTests.cs b/mbproxy/tests/Mbproxy.Tests/Bcd/BcdTagMapBuilderTests.cs index 768728a..957dc21 100644 --- a/mbproxy/tests/Mbproxy.Tests/Bcd/BcdTagMapBuilderTests.cs +++ b/mbproxy/tests/Mbproxy.Tests/Bcd/BcdTagMapBuilderTests.cs @@ -98,62 +98,71 @@ public sealed class BcdTagMapBuilderTests tag.Width.ShouldBe((byte)32); } + /// + /// Phase 12 (W2.11) — duplicates within Global itself are now detected + /// pre-collapse and produce a DuplicateAddress error. (Before W2.11 the input + /// dictionary silently collapsed to last-write-wins, leaving the validator dead.) + /// [Fact] public void Build_DuplicateAddressInGlobal_ReturnsDuplicateAddressError() { - // Two options with the same address in Global. - // The working dictionary collapses them (last-write-wins), - // so a true duplicate is one in Add that matches Global after step 3 - // has already resolved — which the builder handles as "Add wins" (no error). - // This test instead validates the case where Global has a structural duplicate - // after the full resolution results in one address appearing twice, which can - // happen if the options list is constructed with the same address twice. var global = new BcdTagListOptions { Global = [ new BcdTagOptions { Address = 1072, Width = 16 }, - new BcdTagOptions { Address = 1072, Width = 32 }, // same address, different width + new BcdTagOptions { Address = 1072, Width = 32 }, // duplicate within Global ] }; - // The dictionary collapses to one entry (last-write-wins in the dictionary). - // A real duplicate-detection scenario: two separately-identical entries through Add. - // Let's construct a true duplicate through the Add path overwriting Global - // and then adding the same address again. - // Actually: our builder uses Dictionary which deduplicates - // by key. The DuplicateAddress error fires when seenAddresses already contains addr, - // which can only happen if working has two entries with the same key — but Dictionary - // prevents that. The correct scenario is: two Add entries with the same address in - // the IReadOnlyList (list allows duplication even though dict collapses them). - // Since the builder iterates the list and adds to dict, duplicates in the list - // get silently resolved. The DuplicateAddress error is thus for a theoretical - // future path; let's verify the "Add with same address as existing" path instead. var result = BcdTagMapBuilder.Build(global, perPlc: null); - // Should resolve cleanly (dict collapses to last write). - result.Errors.ShouldBeEmpty(); - result.Map.Count.ShouldBe(1); + result.Errors.ShouldContain(e => e.Kind == BcdValidationError.DuplicateAddress + && e.Address == 1072); } + /// + /// Phase 12 (W2.11) — duplicates within the per-PLC Add list itself are now detected + /// pre-collapse. (Cross-list collisions Global vs Add remain the legitimate width- + /// override pattern and are NOT errors — see the next test.) + /// [Fact] - public void Build_DuplicateAddress_Via_AddList_Produces_No_Error_LastWriteWins() + public void Build_DuplicateAddress_Within_AddList_ReturnsDuplicateAddressError() { - // The Add list has two entries for the same address; builder sees the last one. - // This is intentional: it allows width overrides. No duplicate error expected. var global = Global((1072, 16)); var perPlc = new PlcBcdOverrides { Add = [ - new BcdTagOptions { Address = 1072, Width = 16 }, - new BcdTagOptions { Address = 1072, Width = 32 }, // override the first Add + new BcdTagOptions { Address = 1080, Width = 16 }, + new BcdTagOptions { Address = 1080, Width = 32 }, // duplicate within Add ], Remove = [], }; var result = BcdTagMapBuilder.Build(global, perPlc); + result.Errors.ShouldContain(e => e.Kind == BcdValidationError.DuplicateAddress + && e.Address == 1080); + } + + /// + /// Phase 12 (W2.11) — same-address entries appearing in BOTH Global AND Add are + /// the documented width-override pattern (design.md "Hybrid tag resolution"). They + /// must NOT be flagged as duplicates; Add wins. + /// + [Fact] + public void Build_AddOverridesGlobalAtSameAddress_NoDuplicateError_AddWins() + { + var global = Global((1072, 16)); + var perPlc = new PlcBcdOverrides + { + Add = [ new BcdTagOptions { Address = 1072, Width = 32 } ], + Remove = [], + }; + + var result = BcdTagMapBuilder.Build(global, perPlc); + result.Errors.ShouldBeEmpty(); result.Map.TryGet(1072, out var tag).ShouldBeTrue(); tag.Width.ShouldBe((byte)32); diff --git a/mbproxy/tests/Mbproxy.Tests/Configuration/ConfigReconcilerTests.cs b/mbproxy/tests/Mbproxy.Tests/Configuration/ConfigReconcilerTests.cs index 6baf05f..a8ea22b 100644 --- a/mbproxy/tests/Mbproxy.Tests/Configuration/ConfigReconcilerTests.cs +++ b/mbproxy/tests/Mbproxy.Tests/Configuration/ConfigReconcilerTests.cs @@ -109,7 +109,7 @@ public sealed class ConfigReconcilerTests : IAsyncDisposable _supervisors.Add(supA); await supA.StartAsync(CancellationToken.None); - var supervisors = new Dictionary(StringComparer.Ordinal) + var supervisors = new System.Collections.Concurrent.ConcurrentDictionary(StringComparer.Ordinal) { ["A"] = supA, }; @@ -149,7 +149,7 @@ public sealed class ConfigReconcilerTests : IAsyncDisposable _supervisors.Add(supA); await supA.StartAsync(CancellationToken.None); - var supervisors = new Dictionary(StringComparer.Ordinal) + var supervisors = new System.Collections.Concurrent.ConcurrentDictionary(StringComparer.Ordinal) { ["A"] = supA, }; @@ -207,7 +207,7 @@ public sealed class ConfigReconcilerTests : IAsyncDisposable await supA.WaitForInitialBindAttemptAsync(waitCts.Token); Assert.Equal(SupervisorState.Bound, supA.Snapshot().State); - var supervisors = new Dictionary(StringComparer.Ordinal) + var supervisors = new System.Collections.Concurrent.ConcurrentDictionary(StringComparer.Ordinal) { ["A"] = supA, }; @@ -245,7 +245,7 @@ public sealed class ConfigReconcilerTests : IAsyncDisposable var counters = new ServiceCounters(); var reconciler = BuildReconciler(monitor, counters); _reconcilers.Add(reconciler); - reconciler.Attach(new Dictionary(StringComparer.Ordinal), initial); + reconciler.Attach(new System.Collections.Concurrent.ConcurrentDictionary(StringComparer.Ordinal), initial); // Fire 5 concurrent Apply calls — they must execute one-at-a-time. var opts = MakeOptions([]); diff --git a/mbproxy/tests/Mbproxy.Tests/Configuration/ReloadValidatorTests.cs b/mbproxy/tests/Mbproxy.Tests/Configuration/ReloadValidatorTests.cs index dbd437a..a49e23c 100644 --- a/mbproxy/tests/Mbproxy.Tests/Configuration/ReloadValidatorTests.cs +++ b/mbproxy/tests/Mbproxy.Tests/Configuration/ReloadValidatorTests.cs @@ -155,4 +155,113 @@ public sealed class ReloadValidatorTests Assert.False(valid); Assert.Contains(errors, e => e.Contains("non-empty")); } + + // ── Phase 12 (W2.10) — Cache.AllowLongTtl gate ────────────────────────────────────── + + /// + /// W2 — per-tag CacheTtlMs > 60_000 without Cache.AllowLongTtl is rejected. + /// + [Fact] + public void Validate_PerTagCacheTtl_Above60s_Without_AllowLongTtl_Fails() + { + var opts = new MbproxyOptions + { + Plcs = [MakePlc("PLC-A", 5020)], + BcdTags = new BcdTagListOptions + { + Global = [ new BcdTagOptions { Address = 1024, Width = 16, CacheTtlMs = 120_000 } ], + }, + Cache = new CacheOptions { AllowLongTtl = false }, + }; + + bool valid = ReloadValidator.Validate(opts, out var errors); + + Assert.False(valid); + Assert.Contains(errors, e => e.Contains("AllowLongTtl") && e.Contains("60_000")); + } + + /// + /// W2 — same value passes when AllowLongTtl is true (operator opt-in). + /// + [Fact] + public void Validate_PerTagCacheTtl_Above60s_With_AllowLongTtl_Passes() + { + var opts = new MbproxyOptions + { + Plcs = [MakePlc("PLC-A", 5020)], + BcdTags = new BcdTagListOptions + { + Global = [ new BcdTagOptions { Address = 1024, Width = 16, CacheTtlMs = 120_000 } ], + }, + Cache = new CacheOptions { AllowLongTtl = true }, + }; + + bool valid = ReloadValidator.Validate(opts, out var errors); + + Assert.True(valid); + Assert.Empty(errors); + } + + /// + /// W2 — per-PLC DefaultCacheTtlMs > 60_000 inherited by a tag with null CacheTtlMs is + /// caught by the resolved-value check even if the per-PLC default check itself passes + /// (it doesn't, but this validates the defensive resolved re-check from W2.10). + /// + [Fact] + public void Validate_ResolvedTtl_FromPerPlcDefault_AboveCap_Fails() + { + var opts = new MbproxyOptions + { + Plcs = [ + new PlcOptions + { + Name = "PLC-A", ListenPort = 5020, Host = "127.0.0.1", Port = 502, + DefaultCacheTtlMs = 90_000, + }, + ], + BcdTags = new BcdTagListOptions + { + // Tag with no explicit CacheTtlMs — inherits the per-PLC 90_000. + Global = [ new BcdTagOptions { Address = 1024, Width = 16 } ], + }, + Cache = new CacheOptions { AllowLongTtl = false }, + }; + + bool valid = ReloadValidator.Validate(opts, out var errors); + + Assert.False(valid); + Assert.Contains(errors, e => e.Contains("60_000")); + } + + // ── Phase 12 (W2.18) — ConnectionOptions validation ───────────────────────────────── + + [Fact] + public void Validate_ZeroBackendConnectTimeoutMs_Fails() + { + var opts = new MbproxyOptions + { + Plcs = [MakePlc("PLC-A", 5020)], + Connection = new ConnectionOptions { BackendConnectTimeoutMs = 0 }, + }; + + bool valid = ReloadValidator.Validate(opts, out var errors); + + Assert.False(valid); + Assert.Contains(errors, e => e.Contains("BackendConnectTimeoutMs")); + } + + [Fact] + public void Validate_NegativeGracefulShutdownTimeoutMs_Fails() + { + var opts = new MbproxyOptions + { + Plcs = [MakePlc("PLC-A", 5020)], + Connection = new ConnectionOptions { GracefulShutdownTimeoutMs = -1 }, + }; + + bool valid = ReloadValidator.Validate(opts, out var errors); + + Assert.False(valid); + Assert.Contains(errors, e => e.Contains("GracefulShutdownTimeoutMs")); + } } diff --git a/mbproxy/tests/Mbproxy.Tests/Mbproxy.Tests.csproj b/mbproxy/tests/Mbproxy.Tests/Mbproxy.Tests.csproj index 8c551b3..9a618a3 100644 --- a/mbproxy/tests/Mbproxy.Tests/Mbproxy.Tests.csproj +++ b/mbproxy/tests/Mbproxy.Tests/Mbproxy.Tests.csproj @@ -31,4 +31,12 @@ + + + + + diff --git a/mbproxy/tests/Mbproxy.Tests/Proxy/BcdPduPipelineTests.cs b/mbproxy/tests/Mbproxy.Tests/Proxy/BcdPduPipelineTests.cs index c1eaa8c..9f25333 100644 --- a/mbproxy/tests/Mbproxy.Tests/Proxy/BcdPduPipelineTests.cs +++ b/mbproxy/tests/Mbproxy.Tests/Proxy/BcdPduPipelineTests.cs @@ -360,6 +360,50 @@ public sealed class BcdPduPipelineTests ctx.Counters.Snapshot().RewrittenSlots.ShouldBe(2); } + /// + /// Phase 12 (W2.13) — a client writing a 32-bit BCD value where either word exceeds + /// 9999 must NOT be silently mutated by the `high*10000+low` reconstruction. Validation + /// rejects the slot, increments invalidBcdWarnings, and passes the raw bytes through. + /// Without W2.13 the codec would accept e.g. (high=9999, low=9999) → 99_989_999 → + /// re-encode as (high=9998, low=9999), silently losing 1 from the high word. + /// + [Fact] + public void FC16_32Bit_ClientHighOrLowAbove9999_PassesThroughRaw_WithInvalidBcdWarning() + { + var ctx = MakeContext(BcdTag.Create(800, 32)); + // qty=2, low at offset 0, high at offset 1; both at 0xFFFF (= 65535, > 9999). + var pdu = Fc16Request(800, 0xFFFF, 0xFFFF); + byte[] original = [..pdu]; + + Pipeline.Process(MbapDirection.RequestToBackend, ReadOnlySpan.Empty, pdu.AsSpan(), ctx); + + pdu.ShouldBe(original, "OOR client values must pass through raw, not be silently mutated"); + ctx.Counters.Snapshot().InvalidBcdWarnings.ShouldBe(1); + ctx.Counters.Snapshot().RewrittenSlots.ShouldBe(0); + } + + /// + /// Phase 12 (W2.14) — a malformed FC16 request that claims qty=N but ships fewer than + /// 6+N*2 bytes must NOT be partially rewritten. Without W2.14 each individual slot's + /// per-slot bounds check would skip the OOB slot, leaving early slots rewritten and late + /// slots untouched (a half-rewritten request reaching the PLC). + /// + [Fact] + public void FC16_TruncatedRegisterData_PassesThroughRaw_NoPartialRewrite() + { + var ctx = MakeContext(BcdTag.Create(900, 16)); + // Build a normal 1-register write, then trim 1 byte off the end so qty=1 but only + // 1 byte of register data remains. + var pdu = Fc16Request(900, 1234); + byte[] truncated = pdu.AsSpan(0, pdu.Length - 1).ToArray(); + byte[] original = [..truncated]; + + Pipeline.Process(MbapDirection.RequestToBackend, ReadOnlySpan.Empty, truncated.AsSpan(), ctx); + + truncated.ShouldBe(original, "truncated FC16 must pass through raw"); + ctx.Counters.Snapshot().RewrittenSlots.ShouldBe(0); + } + [Fact] public void FC16_WritePartiallyOverlapping32BitPair_PassesThroughRaw_WithPartialWarning() {