From e66b17fe5f37785b233e59c45f400089b083d209 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 14 May 2026 05:48:44 -0400 Subject: [PATCH] mbproxy: Wave 2 fixes from 2026-05-14 code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves the 21 Major findings catalogued in codereviews/2026-05-14/RemediationPlan.md (Wave 2). Tests: 370 pass / 0 fail (baseline 363 + 7 new W2 regression tests). Multiplexer / concurrency: W2.1 ConfigReconciler.Attach now threads the live coalescingAccessor through to add/restart-built supervisors so a hot-reload of ReadCoalescing.{Enabled,MaxParties} propagates to PLCs added or restarted via reload. W2.2 PlcMultiplexer._disposed and UpstreamPipe._disposed are now volatile for ARM/portability defense. W2.3 ProxyWorker._supervisors / ConfigReconciler._supervisors switched from Dictionary to ConcurrentDictionary; reconciler uses TryRemove. The outer Apply is serialised by a semaphore but the inner Add/Remove/ Restart Task.WhenAll continuations run in parallel. W2.4 Counter parity for cache miss + coalescing-saturation miss documented inline (per-design contract; behavior unchanged). W2.5 _disposeCts.Dispose() and _connectGate.Dispose() guarded against late watchdog ticks. W2.6 _connectGate disposed in DisposeAsync. W2.7 Inline doc clarifying the post-rewriter FC byte read. Cache / hot-reload: W2.8 PlcListenerSupervisor.ReplaceContextAsync now calls Clear() to capture the entry count, emits mbproxy.cache.flushed, then disposes the old cache. Previously the event was defined but never emitted. W2.9 Inline doc explaining the implicit "skip cache invalidation while recovering" gating (no backend reader during recovery → no FC06/FC16 response → no invalidation). W2.10 ReloadValidator now re-checks resolved per-tag CacheTtlMs against Cache.AllowLongTtl after BcdTagMapBuilder folds the per-PLC default. BCD rewriter: W2.11 Duplicate addresses detected within Global itself and within the per-PLC Add list itself, BEFORE the working dictionary collapses keys. Cross-list collisions (Global vs Add) remain the documented width-override pattern. Previously the DuplicateAddress error was unreachable dead code. W2.12 OverlappingHighRegister reports each colliding pair exactly once (canonicalised low/high pair tracked in a HashSet). W2.13 FC16 32-bit write rejects clientLow > 9999 or clientHigh > 9999 BEFORE the high*10000+low reconstruction. Without this guard, (high=9999, low=9999) silently re-encoded as (high=9998, low=9999), losing 1 from the high word. W2.14 FC16 validates pdu.Length >= 6 + qty*2 upfront — no half-rewritten requests when a malformed client claims more registers than it ships. Supervisor: W2.15 WaitForInitialBindAttemptAsync now backed by TaskCompletionSource instead of 10ms busy-poll. Resolves race against fast Stopped→Bound→ Stopped transitions and hangs when the supervisor task throws. W2.16 StartAsync refuses re-entry on a non-Stopped supervisor (was leaking the previous _supervisorCts). W2.17 New TransitionTo helper writes _state, _lastBindError, and (optionally) _recoveryAttempts under one lock. Snapshot() reads under the same lock so the status page never reports an inconsistent triple. Truncate helper extracted (was copy-pasted across three sites). W2.18 MbproxyOptionsValidator + ReloadValidator reject Connection.{Backend ConnectTimeoutMs, BackendRequestTimeoutMs, GracefulShutdownTimeoutMs} <= 0. Misconfigured 0 produces immediate CancelAfter(0) failures. Hosting / diagnostics: W2.20 ProxyWorker.StopAsync supervisor-stop deadline now reads from IOptionsMonitor.CurrentValue.Connection.GracefulShutdownTimeoutMs (was hard-coded 5s). W2.21 src/Mbproxy/appsettings.json deleted; the published file is now a Link to install/mbproxy.config.template.json so the binary ships with a usable, fully-commented example config instead of an empty stub. Tests strip the inherited file from their bin via an AfterTargets="Build" Target so they don't pick up the template's example PLCs. W2.22 invalidBcdWarnings (PlcPdusStatus) and codeOther (ExceptionCounts) added to StatusDto, plumbed through StatusSnapshotBuilder, surfaced in StatusHtmlRenderer table cells. W2.23 EventLogBridge caches EventLog.SourceExists at construction so Emit doesn't hit the registry on every Error+ log line. New regression tests: ReloadValidatorTests: Validate_PerTagCacheTtl_Above60s_Without_AllowLongTtl_Fails Validate_PerTagCacheTtl_Above60s_With_AllowLongTtl_Passes Validate_ResolvedTtl_FromPerPlcDefault_AboveCap_Fails Validate_ZeroBackendConnectTimeoutMs_Fails Validate_NegativeGracefulShutdownTimeoutMs_Fails BcdPduPipelineTests: FC16_32Bit_ClientHighOrLowAbove9999_PassesThroughRaw_WithInvalidBcdWarning FC16_TruncatedRegisterData_PassesThroughRaw_NoPartialRewrite Reworked tests in BcdTagMapBuilderTests for the W2.11 contract (Global dup, Add dup, Add-overrides-Global accepted as width override). Co-Authored-By: Claude Opus 4.7 (1M context) --- mbproxy/src/Mbproxy/Admin/StatusDto.cs | 15 +- .../src/Mbproxy/Admin/StatusHtmlRenderer.cs | 4 +- .../Mbproxy/Admin/StatusSnapshotBuilder.cs | 6 +- mbproxy/src/Mbproxy/Bcd/BcdTagMapBuilder.cs | 46 ++++-- .../Mbproxy/Configuration/ConfigReconciler.cs | 50 ++++--- .../Mbproxy/Configuration/ReloadValidator.cs | 28 ++++ .../src/Mbproxy/Diagnostics/EventLogBridge.cs | 15 +- mbproxy/src/Mbproxy/Mbproxy.csproj | 10 +- mbproxy/src/Mbproxy/Options/MbproxyOptions.cs | 13 ++ mbproxy/src/Mbproxy/Proxy/BcdPduPipeline.cs | 26 +++- .../Proxy/Multiplexing/PlcMultiplexer.cs | 41 +++++- .../Proxy/Multiplexing/UpstreamPipe.cs | 4 +- mbproxy/src/Mbproxy/Proxy/ProxyWorker.cs | 26 +++- .../Supervision/PlcListenerSupervisor.cs | 132 ++++++++++++++---- mbproxy/src/Mbproxy/appsettings.json | 54 ------- .../Admin/StatusHtmlRendererTests.cs | 4 +- .../Bcd/BcdTagMapBuilderTests.cs | 65 +++++---- .../Configuration/ConfigReconcilerTests.cs | 8 +- .../Configuration/ReloadValidatorTests.cs | 109 +++++++++++++++ .../tests/Mbproxy.Tests/Mbproxy.Tests.csproj | 8 ++ .../Proxy/BcdPduPipelineTests.cs | 44 ++++++ 21 files changed, 545 insertions(+), 163 deletions(-) delete mode 100644 mbproxy/src/Mbproxy/appsettings.json 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() {