diff --git a/mbproxy/README.md b/mbproxy/README.md index 09f7f17..4739881 100644 --- a/mbproxy/README.md +++ b/mbproxy/README.md @@ -2,6 +2,8 @@ A .NET 10 Windows Service that sits inline as a Modbus TCP proxy in front of a fleet of AutomationDirect DirectLOGIC DL205/DL260 controllers, rewriting BCD-encoded registers bidirectionally so upstream clients can read and write them as plain integers. The proxy also offers an opt-in per-tag response cache (default OFF) for FC03/FC04 reads with bounded operator-configured staleness — see [`docs/Architecture/ResponseCache.md`](docs/Architecture/ResponseCache.md) before enabling it. +> ⚠ **32-bit BCD wire format is "two base-10000 digits in CDAB", not standard CDAB binary Int32.** A 32-bit BCD tag at address `A` decodes as `decimal = high * 10_000 + low` where `low` is the register at `A` and `high` is the register at `A+1`. Each word independently must be 0–9999. Standard Modbus clients (NModbus, FluentModbus, Wonderware DAServer) that interpret CDAB as straight binary Int32 will silently corrupt any value > 9999 on writes and read garbage on reads. Configure your client to send/receive each register as a separate base-10000 BCD digit pair, not as a single binary Int32. Full details in [`docs/Features/BcdRewriting.md`](docs/Features/BcdRewriting.md). + ## Hard constraints / prerequisites - **Windows 10 / Server 2019 or later, 64-bit.** No Linux or Docker support — the service uses `Microsoft.Extensions.Hosting.WindowsServices` and the Windows Event Log. diff --git a/mbproxy/docs/Architecture/ResponseCache.md b/mbproxy/docs/Architecture/ResponseCache.md index 420e0b5..9a47202 100644 --- a/mbproxy/docs/Architecture/ResponseCache.md +++ b/mbproxy/docs/Architecture/ResponseCache.md @@ -303,6 +303,17 @@ cached read is still consistent with the device's actual state. Skipping the invalidation matches reality — the write did not take effect, so the read is not stale. +The skip is **structural**, not conditional. Cache invalidation only +fires inside the per-PLC backend reader task, after a non-exception +FC06/FC16 response arrives from the PLC. A `recovering` supervisor has +torn down its multiplexer and there is no backend reader, so no response +can land and the invalidation path is never entered. This is the +reasoning the code at `Proxy/Multiplexing/PlcMultiplexer.cs` documents +inline (W2.9). If a future change ever produced a write response off the +live backend (e.g. a mocked-response path), an explicit `Recovering` +check would need to be added at the invalidator call site to keep the +skip semantics correct. + ## No Persistence The cache is purely in-memory. Process restart wipes every entry. There diff --git a/mbproxy/docs/Operations/Configuration.md b/mbproxy/docs/Operations/Configuration.md index d5a1907..74d3aba 100644 --- a/mbproxy/docs/Operations/Configuration.md +++ b/mbproxy/docs/Operations/Configuration.md @@ -86,6 +86,8 @@ Every supported key under `Mbproxy:*`, populated to a representative default: `Serilog` configuration is documented in [`./Troubleshooting.md`](./Troubleshooting.md) and lives outside the `Mbproxy` section. +> The Windows Event Log sink is **not** the standard `Serilog.Sinks.EventLog` package. It is a custom `EventLogBridge` (`src/Mbproxy/Diagnostics/EventLogBridge.cs`) that writes Error+ events to the `mbproxy` source under `Application` only when the service runs under the SCM. Event Log source registration is intentionally NOT attempted at runtime (the service account may not be admin); `install.ps1` registers the source at install time. Don't add `Serilog.Sinks.EventLog` — the bridge would duplicate every event. The bridge caches the source-exists check at construction (Phase 12 / W2.23), so a missing source produces no per-event registry traffic. + ## `Mbproxy.AdminPort` Port for the read-only HTTP status server. Binds to all interfaces on startup. diff --git a/mbproxy/docs/design.md b/mbproxy/docs/design.md index 94483f3..45c7378 100644 --- a/mbproxy/docs/design.md +++ b/mbproxy/docs/design.md @@ -136,7 +136,7 @@ Properties: - **Hot-reloadable on/off.** `Mbproxy.Resilience.ReadCoalescing.Enabled` defaults to `true`. Flipping it to `false` at runtime leaves running coalesced entries to drain naturally; subsequent FC03/04 requests take the Phase-9 (one round-trip per upstream request) path. - **Transparency contract preserved.** Each upstream client still sees its own original MBAP TxId on the response. The BCD rewriter runs once on the shared response buffer; per-party copies are only made when fan-out has more than one party. -Counter accounting balance (per snapshot): `coalescedHitCount + coalescedMissCount` equals the total FC03 + FC04 requests seen since the multiplexer was constructed. Both counters increment regardless of whether the coalescing feature is enabled — `coalescedHitCount` is 0 when disabled, but every read still increments `coalescedMissCount`. +Counter accounting balance (per snapshot): `coalescedHitCount + coalescedMissCount` equals the total FC03 + FC04 requests seen since the multiplexer was constructed. Both counters increment regardless of whether the coalescing feature is enabled — `coalescedHitCount` is 0 when disabled, but every read still increments `coalescedMissCount`. **Saturation paths (allocator full, duplicate-key race) also count as a miss** even though they produce no backend round-trip — the identity above is preserved by counting every entry into the coalescing path, not every backend send. Operators wanting "actual backend round-trips opened" should subtract the multiplexer's exception-04 frames produced from saturation. ## Response cache (Phase 11) — opt-in bounded-staleness cache @@ -173,7 +173,7 @@ The BCD rewriter runs once on the cache-miss path (the backend reader task decod ### Counter accounting - `cacheHitCount` — FC03/FC04 requests served from the cache. -- `cacheMissCount` — FC03/FC04 requests that fell through to the coalescing/backend path. (Cache hit + Cache miss = total FC03/FC04 requests that were cache-eligible, i.e. whose resolved TTL was > 0; reads whose effective TTL is 0 increment neither.) +- `cacheMissCount` — FC03/FC04 requests that fell through to the coalescing/backend path. (Cache hit + Cache miss = total FC03/FC04 requests that were cache-eligible, i.e. whose resolved TTL was > 0; reads whose effective TTL is 0 increment neither.) **A "miss" does NOT mean "produced a backend round-trip."** Two upstream peers issuing the same cache-eligible read both increment `cacheMissCount`; one then opens a backend round-trip and the other coalesces onto it via the InFlightByKey path (incrementing `coalescedHitCount`). Operators reading these counters as "backend reads opened" should use `cacheMissCount − coalescedHitCount` as the lower bound on actual backend traffic. - `cacheInvalidations` — count of cache entries invalidated by FC06/FC16 write responses. - `cacheEntryCount` — point-in-time snapshot of `ResponseCache.Count` (Tier-2 memory-watch KPI). - `cacheBytes` — point-in-time approximation of cached PDU bytes (Tier-2 memory-watch KPI). diff --git a/mbproxy/docs/plan/README.md b/mbproxy/docs/plan/README.md index 413e04e..d3228ec 100644 --- a/mbproxy/docs/plan/README.md +++ b/mbproxy/docs/plan/README.md @@ -20,6 +20,7 @@ Phase-by-phase implementation plan for the `mbproxy` service. Each phase is a se | 09 | [TxId multiplexing](09-txid-multiplexing.md) — single backend connection per PLC (post-1.0 follow-on) | 04, 05, 07 | — | | 10 | [Read coalescing](10-read-coalescing.md) — in-flight FC03/04 dedup (post-1.0 follow-on) | 09 | — | | 11 | [Response cache](11-response-cache.md) — short-TTL post-response cache, bounded staleness (post-1.0; **design-contract pivot**) | 10 | — | +| 12 | Code-review remediation (2026-05-14) — Wave 1 critical, Wave 2 major, Wave 3 cleanup. Plan and findings in [`../../codereviews/2026-05-14/`](../../codereviews/2026-05-14/RemediationPlan.md). | 11 | — | ``` ┌── 01 (sim) ──┐ diff --git a/mbproxy/src/Mbproxy/Admin/StatusSnapshotBuilder.cs b/mbproxy/src/Mbproxy/Admin/StatusSnapshotBuilder.cs index 6a2ad5e..83f1bc1 100644 --- a/mbproxy/src/Mbproxy/Admin/StatusSnapshotBuilder.cs +++ b/mbproxy/src/Mbproxy/Admin/StatusSnapshotBuilder.cs @@ -66,7 +66,11 @@ internal sealed class StatusSnapshotBuilder var activeUpstreams = supervisor?.ActiveUpstreams ?? Array.Empty(); var clientSnapshots = activeUpstreams .Select(p => new ClientSnapshot( - Remote: p.RemoteEp?.ToString() ?? p.RemoteEp?.Address.ToString() ?? "?", + // Phase 12 (W3 cleanup) — the second `?.Address.ToString()` was + // unreachable: if RemoteEp is non-null the first ?.ToString() returns + // a string; if it's null the second branch's outer `?.` short-circuits + // identically. Simplified to the equivalent two-branch form. + Remote: p.RemoteEp?.ToString() ?? "?", ConnectedAtUtc: p.ConnectedAtUtc, PdusForwarded: p.PdusForwardedCount)) .ToList(); diff --git a/mbproxy/src/Mbproxy/Bcd/BcdCodec.cs b/mbproxy/src/Mbproxy/Bcd/BcdCodec.cs index 7e74369..122bb88 100644 --- a/mbproxy/src/Mbproxy/Bcd/BcdCodec.cs +++ b/mbproxy/src/Mbproxy/Bcd/BcdCodec.cs @@ -97,15 +97,18 @@ internal static class BcdCodec return hiVal * 10_000 + loVal; } - // ── Private helpers ───────────────────────────────────────────────────── + // ── Helpers ───────────────────────────────────────────────────────────── - /// Returns true if any nibble in is >= 0xA. - private static bool HasBadNibble(ushort raw) + /// + /// Returns true if any nibble in is >= 0xA (i.e. a non-BCD + /// digit). Internal so can call it from + /// the response-rewrite path's per-word check without re-implementing the same logic. + /// + internal static bool HasBadNibble(ushort raw) { - // Check each nibble independently. return ((raw >> 12) & 0xF) >= 0xA - || ((raw >> 8) & 0xF) >= 0xA - || ((raw >> 4) & 0xF) >= 0xA - || (raw & 0xF) >= 0xA; + || ((raw >> 8) & 0xF) >= 0xA + || ((raw >> 4) & 0xF) >= 0xA + || (raw & 0xF) >= 0xA; } } diff --git a/mbproxy/src/Mbproxy/Mbproxy.csproj b/mbproxy/src/Mbproxy/Mbproxy.csproj index ce110a0..85def22 100644 --- a/mbproxy/src/Mbproxy/Mbproxy.csproj +++ b/mbproxy/src/Mbproxy/Mbproxy.csproj @@ -39,7 +39,8 @@ - + diff --git a/mbproxy/src/Mbproxy/Proxy/BcdPduPipeline.cs b/mbproxy/src/Mbproxy/Proxy/BcdPduPipeline.cs index 1c2448d..2486be1 100644 --- a/mbproxy/src/Mbproxy/Proxy/BcdPduPipeline.cs +++ b/mbproxy/src/Mbproxy/Proxy/BcdPduPipeline.cs @@ -385,8 +385,8 @@ internal sealed class BcdPduPipeline : IPduPipeline catch (FormatException) { // Emit invalid_bcd for the low register (first bad word we'd encounter). - ushort badRaw = HasBadNibble(rawLow) ? rawLow : rawHigh; - ushort badAddr = HasBadNibble(rawLow) ? tag.Address : tag.HighRegister; + ushort badRaw = BcdCodec.HasBadNibble(rawLow) ? rawLow : rawHigh; + ushort badAddr = BcdCodec.HasBadNibble(rawLow) ? tag.Address : tag.HighRegister; RewriterLogEvents.InvalidBcd(ctx.Logger, ctx.PlcName, badAddr, badRaw, "Read"); ctx.Counters.IncrementInvalidBcd(); continue; @@ -473,12 +473,6 @@ internal sealed class BcdPduPipeline : IPduPipeline // already counted this slot on the way out. Incrementing again would double-count. } - // ── Helpers ────────────────────────────────────────────────────────────── - - /// Returns true if any nibble of is >= 0xA. - private static bool HasBadNibble(ushort raw) - => ((raw >> 12) & 0xF) >= 0xA - || ((raw >> 8) & 0xF) >= 0xA - || ((raw >> 4) & 0xF) >= 0xA - || (raw & 0xF) >= 0xA; + // Phase 12 (W3 cleanup) — HasBadNibble was previously duplicated here; the canonical + // implementation now lives in BcdCodec.HasBadNibble (internal). } diff --git a/mbproxy/src/Mbproxy/Proxy/Multiplexing/InFlightByKeyMap.cs b/mbproxy/src/Mbproxy/Proxy/Multiplexing/InFlightByKeyMap.cs index c53d0b1..1f0456d 100644 --- a/mbproxy/src/Mbproxy/Proxy/Multiplexing/InFlightByKeyMap.cs +++ b/mbproxy/src/Mbproxy/Proxy/Multiplexing/InFlightByKeyMap.cs @@ -56,10 +56,11 @@ internal sealed class InFlightByKeyMap /// a fresh entry (and a fresh backend round-trip). This bounds the response-fanout /// cost per entry at O(maxParties). /// - /// Returns true always (the bool return matches the phase doc's signature; - /// future evolution could introduce a refusal path). + /// Phase 12 (W3 cleanup) — was previously declared as bool TryAttachOrCreate + /// but always returned true. The bool was dead; the result is in the + /// out parameter. /// - public bool TryAttachOrCreate( + public void AttachOrCreate( CoalescingKey key, InterestedParty party, Func factory, @@ -76,13 +77,12 @@ internal sealed class InFlightByKeyMap existingList.Add(party); req = existing; wasNew = false; - return true; + return; } req = factory(); _entries[key] = req; wasNew = true; - return true; } } diff --git a/mbproxy/src/Mbproxy/Proxy/Multiplexing/PlcMultiplexer.cs b/mbproxy/src/Mbproxy/Proxy/Multiplexing/PlcMultiplexer.cs index 7e18a7c..370d489 100644 --- a/mbproxy/src/Mbproxy/Proxy/Multiplexing/PlcMultiplexer.cs +++ b/mbproxy/src/Mbproxy/Proxy/Multiplexing/PlcMultiplexer.cs @@ -508,11 +508,9 @@ internal sealed class PlcMultiplexer : IAsyncDisposable, IMultiplexCountersProvi _inFlightByKey.TryRemove(coalKey, out _); } - // Update EWMA round-trip from when we sent the request. - long elapsedMs = (DateTimeOffset.UtcNow - inFlight.SentAtUtc).Ticks * 100; // 100 ns per tick - // UpdateRoundTripEwma expects Stopwatch ticks, but we have wall-clock. - // Convert ms back to Stopwatch ticks: - long ticks = (long)((double)(DateTimeOffset.UtcNow - inFlight.SentAtUtc).TotalSeconds * Stopwatch.Frequency); + // Update EWMA round-trip from when we sent the request. UpdateRoundTripEwma + // expects Stopwatch ticks; convert from the wall-clock SentAtUtc timestamp. + long ticks = (long)((DateTimeOffset.UtcNow - inFlight.SentAtUtc).TotalSeconds * Stopwatch.Frequency); if (ticks > 0) _ctx.Counters.UpdateRoundTripEwma(ticks); @@ -756,7 +754,7 @@ internal sealed class PlcMultiplexer : IAsyncDisposable, IMultiplexCountersProvi ushort proxyTxIdForSend = 0; InFlightRequest? inFlightForSend = null; - _inFlightByKey.TryAttachOrCreate( + _inFlightByKey.AttachOrCreate( key, newParty, factory: () => @@ -970,7 +968,7 @@ internal sealed class PlcMultiplexer : IAsyncDisposable, IMultiplexCountersProvi private async Task RunRequestTimeoutWatchdogAsync(CancellationToken ct) { // Tick at ~quarter of the request timeout for responsive cleanup, but cap to a - // 1-second floor so the watchdog doesn't busy-wake on very small timeouts. + // 100 ms floor so the watchdog doesn't busy-wake on very small timeouts. int tickMs = Math.Max(100, _connectionOptions.BackendRequestTimeoutMs / 4); try diff --git a/mbproxy/src/Mbproxy/Proxy/Multiplexing/UpstreamPipe.cs b/mbproxy/src/Mbproxy/Proxy/Multiplexing/UpstreamPipe.cs index e1f58e2..e735204 100644 --- a/mbproxy/src/Mbproxy/Proxy/Multiplexing/UpstreamPipe.cs +++ b/mbproxy/src/Mbproxy/Proxy/Multiplexing/UpstreamPipe.cs @@ -272,8 +272,6 @@ internal sealed partial class UpstreamPipe : IAsyncDisposable Socket socket, byte[] buf, int offset, int count, CancellationToken ct) { int remaining = count; - bool firstRead = true; - while (remaining > 0) { int received = await socket.ReceiveAsync( @@ -281,11 +279,11 @@ internal sealed partial class UpstreamPipe : IAsyncDisposable SocketFlags.None, ct).ConfigureAwait(false); + // Clean EOF (pre-frame or mid-frame) — caller treats both the same. if (received == 0) - return firstRead && remaining == count ? false : false; + return false; remaining -= received; - firstRead = false; } return true; diff --git a/mbproxy/tests/Mbproxy.Tests/Admin/AdminEndpointTests.cs b/mbproxy/tests/Mbproxy.Tests/Admin/AdminEndpointTests.cs index 598e083..91aab17 100644 --- a/mbproxy/tests/Mbproxy.Tests/Admin/AdminEndpointTests.cs +++ b/mbproxy/tests/Mbproxy.Tests/Admin/AdminEndpointTests.cs @@ -330,6 +330,40 @@ public sealed class AdminEndpointTests System.IO.File.Move(tmp, path, overwrite: true); } + // ── Phase 12 (W3 test gap) — non-GET methods rejected ────────────────── + + /// + /// W3 — verifies the admin endpoint rejects non-GET methods (POST / PUT / DELETE) + /// with HTTP 405 Method Not Allowed. The design intentionally exposes only `GET /` + /// and `GET /status.json`; this test guards against an accidental MapPost/Map* being + /// added later. + /// + [Theory(Timeout = 5_000)] + [InlineData("POST")] + [InlineData("PUT")] + [InlineData("DELETE")] + [InlineData("PATCH")] + public async Task NonGetMethod_AgainstAdminRoutes_Returns405(string method) + { + int adminPort = PickFreePort(); + int proxyPort = PickFreePort(); + + var host = BuildHost(adminPort: adminPort, simHost: "127.0.0.1", simPort: 502, + proxyPort: proxyPort, bcd16Addresses: []); + await using var _ = new AsyncHostDispose(host); + await host.StartAsync(TestContext.Current.CancellationToken); + await WaitForAdminAsync(adminPort); + + foreach (string path in new[] { "/", "/status.json" }) + { + using var req = new HttpRequestMessage(new HttpMethod(method), + $"http://127.0.0.1:{adminPort}{path}"); + using var resp = await HttpClient.SendAsync(req, TestContext.Current.CancellationToken); + resp.StatusCode.ShouldBe(HttpStatusCode.MethodNotAllowed, + $"{method} {path} must be rejected (admin endpoint is read-only)"); + } + } + // ── Helpers ─────────────────────────────────────────────────────────────── private static IHost BuildHost( diff --git a/mbproxy/tests/Mbproxy.Tests/Configuration/HotReloadE2ETests.cs b/mbproxy/tests/Mbproxy.Tests/Configuration/HotReloadE2ETests.cs index 9e64fb1..3027874 100644 --- a/mbproxy/tests/Mbproxy.Tests/Configuration/HotReloadE2ETests.cs +++ b/mbproxy/tests/Mbproxy.Tests/Configuration/HotReloadE2ETests.cs @@ -320,6 +320,67 @@ public sealed class HotReloadE2ETests : IAsyncLifetime await host.StopAsync(stopCts.Token); } + // ── Phase 12 (W3 test gap) — cache flush on tag-list reload ───────────────────────── + + /// + /// W2.8 / W3 — verifies that a tag-list reload for a PLC with a cacheable tag emits + /// mbproxy.cache.flushed. The cache count is 0 (no real backend to populate + /// it), but the event must still fire — it's the operator's signal that the in-memory + /// cache state was reset by a config reload. + /// + [Fact(Timeout = 8_000)] + public async Task E2E_TagListReload_OnCacheablePlc_EmitsCacheFlushedEvent() + { + int port = PickFreePort(); + int adminPort = PickFreePort(); + + WriteConfigWithCacheableTag(_configPath, port, adminPort, address: 1024, cacheTtlMs: 60_000); + + var sink = new HotReloadCapturingSink(); + using var host = BuildHost(_configPath, logSink: sink); + using var startCts = new CancellationTokenSource(TimeSpan.FromSeconds(10)); + await host.StartAsync(startCts.Token); + await WaitForAsync(() => CanConnect(port), TimeSpan.FromSeconds(5), + "listener should be reachable after startup"); + + // Mutate the tag list (different address, still cacheable) — this is a Reseat, + // not an Add/Remove, so ReplaceContextAsync runs and the cache flush fires. + WriteConfigWithCacheableTag(_configPath, port, adminPort, address: 1080, cacheTtlMs: 60_000); + + // First confirm the reconciler actually applied the reload at all — gives a clearer + // failure mode than a bare timeout if Reseat isn't firing. + await WaitForAsync( + () => sink.Events.Any(e => e.MessageTemplate.Text.Contains("Config reload applied")), + TimeSpan.FromSeconds(5), + "Config reload applied must fire first; verifies reconciler picked up the change"); + + await WaitForAsync( + () => sink.Events.Any(e => e.MessageTemplate.Text.Contains("Cache flushed")), + TimeSpan.FromSeconds(2), + "expected mbproxy.cache.flushed after tag-list reload on a cacheable PLC"); + + using var stopCts = new CancellationTokenSource(TimeSpan.FromSeconds(5)); + await host.StopAsync(stopCts.Token); + } + + private static void WriteConfigWithCacheableTag( + string path, int listenPort, int adminPort, int address, int cacheTtlMs) + { + var doc = new + { + Mbproxy = new + { + AdminPort = adminPort, + BcdTags = new { Global = new[] { new { Address = address, Width = 16, CacheTtlMs = cacheTtlMs } } }, + Plcs = new[] { new { Name = "PLC-A", ListenPort = listenPort, Host = "127.0.0.1", Port = 502 } }, + Connection = new { BackendConnectTimeoutMs = 500, BackendRequestTimeoutMs = 500 }, + }, + }; + string tmp = path + ".tmp"; + File.WriteAllText(tmp, JsonSerializer.Serialize(doc, new JsonSerializerOptions { WriteIndented = true })); + File.Move(tmp, path, overwrite: true); + } + // ── Helpers ─────────────────────────────────────────────────────────────────────────── private static bool CanConnect(int port) diff --git a/mbproxy/tests/Mbproxy.Tests/Proxy/BcdPduPipelineTests.cs b/mbproxy/tests/Mbproxy.Tests/Proxy/BcdPduPipelineTests.cs index 9f25333..bb0c031 100644 --- a/mbproxy/tests/Mbproxy.Tests/Proxy/BcdPduPipelineTests.cs +++ b/mbproxy/tests/Mbproxy.Tests/Proxy/BcdPduPipelineTests.cs @@ -404,6 +404,87 @@ public sealed class BcdPduPipelineTests ctx.Counters.Snapshot().RewrittenSlots.ShouldBe(0); } + /// + /// Phase 12 (W3 test gap) — symmetric inverse of the existing partial-overlap test: + /// the write range starts ON the high register of a 32-bit pair (low word is BEFORE + /// the write range). Must also be passed through raw with a partial warning, not + /// half-rewritten. + /// + [Fact] + public void FC16_WriteStartsOnHighWord_Of32BitPair_PassesThroughRaw_WithPartialWarning() + { + // 32-bit BCD tag at 700/701; write range 701–702 (qty=2). Low (700) is OUT of + // range; high (701) is IN range — partial overlap on the high side. + var ctx = MakeContext(BcdTag.Create(700, 32)); + var pdu = Fc16Request(701, 0xCCCC, 0xDDDD); + byte[] original = [..pdu]; + + Pipeline.Process(MbapDirection.RequestToBackend, ReadOnlySpan.Empty, pdu.AsSpan(), ctx); + + pdu.ShouldBe(original, "high-only partial overlap must pass through raw"); + ctx.Counters.Snapshot().PartialBcdWarnings.ShouldBe(1); + ctx.Counters.Snapshot().RewrittenSlots.ShouldBe(0); + } + + /// + /// Phase 12 (W3 test gap) — mixed slots in a single FC03 read: a 16-bit BCD tag, a + /// 32-bit BCD pair, and an unconfigured register. Each slot should be handled + /// independently — the 16-bit and 32-bit rewritten, the unconfigured register passed + /// through unchanged. + /// + [Fact] + public void FC03_Mixed_16Bit_32Bit_AndNonBcd_InOneRead_OnlyConfiguredSlotsRewritten() + { + // Layout: + // addr 100: 16-bit BCD → wire 0x1234 → decoded 1234 (= 0x04D2 binary) + // addr 101: unconfigured → passes through 0x9999 + // addr 102: 32-bit BCD low → wire 0x5678 (BCD digits 5,6,7,8 → 5678) + // addr 103: 32-bit BCD high→ wire 0x1234 (BCD digits 1,2,3,4 → 1234) + // decoded = 1234*10_000 + 5678 = 12_345_678 + // emitted as base-10000 binary CDAB: + // low = 12_345_678 % 10_000 = 5678 (binary 0x162E) + // high = 12_345_678 / 10_000 = 1234 (binary 0x04D2) + var ctx = MakeContext(BcdTag.Create(100, 16), BcdTag.Create(102, 32)); + var inFlight = MakeInFlight(0x03, startAddress: 100, qty: 4); + var responseCtx = ctx.WithCurrentRequest(inFlight); + + var pdu = Fc03Response(0x1234, 0x9999, 0x5678, 0x1234); + + Pipeline.Process(MbapDirection.ResponseToClient, ReadOnlySpan.Empty, pdu.AsSpan(), responseCtx); + + // pdu[0]=fc, pdu[1]=byteCount, pdu[2..] = register bytes (2 per register). + ushort reg100 = (ushort)((pdu[2] << 8) | pdu[3]); + ushort reg101 = (ushort)((pdu[4] << 8) | pdu[5]); + ushort reg102 = (ushort)((pdu[6] << 8) | pdu[7]); + ushort reg103 = (ushort)((pdu[8] << 8) | pdu[9]); + + reg100.ShouldBe((ushort)1234, "16-bit BCD slot must decode to 1234"); + reg101.ShouldBe((ushort)0x9999, "unconfigured register must pass through unchanged"); + reg102.ShouldBe((ushort)5678, "32-bit pair low must emit decimal 5678 as binary"); + reg103.ShouldBe((ushort)1234, "32-bit pair high must emit decimal 1234 as binary"); + // Slot count: 1 from 16-bit + 2 from 32-bit pair = 3 rewritten slots. + ctx.Counters.Snapshot().RewrittenSlots.ShouldBe(3); + } + + /// + /// Phase 12 (W3 test gap) — FC16 response handling. The response carries no register + /// values (just an echo of [fc][start][qty]) so the rewriter must pass it through + /// unchanged regardless of tag-map content. + /// + [Fact] + public void FC16_Response_PassesThroughUnchanged_RegardlessOfTagMap() + { + var ctx = MakeContext(BcdTag.Create(100, 16), BcdTag.Create(200, 32)); + // FC16 response: [fc=10][startHi][startLo][qtyHi][qtyLo] = 5 bytes total. + var pdu = new byte[] { 0x10, 0x00, 0x64, 0x00, 0x05 }; + byte[] original = [..pdu]; + + Pipeline.Process(MbapDirection.ResponseToClient, ReadOnlySpan.Empty, pdu.AsSpan(), ctx); + + pdu.ShouldBe(original, "FC16 response carries no register data and must pass through"); + ctx.Counters.Snapshot().RewrittenSlots.ShouldBe(0); + } + [Fact] public void FC16_WritePartiallyOverlapping32BitPair_PassesThroughRaw_WithPartialWarning() { diff --git a/mbproxy/tests/Mbproxy.Tests/Proxy/Multiplexing/InFlightByKeyMapTests.cs b/mbproxy/tests/Mbproxy.Tests/Proxy/Multiplexing/InFlightByKeyMapTests.cs index 000a744..48c3a54 100644 --- a/mbproxy/tests/Mbproxy.Tests/Proxy/Multiplexing/InFlightByKeyMapTests.cs +++ b/mbproxy/tests/Mbproxy.Tests/Proxy/Multiplexing/InFlightByKeyMapTests.cs @@ -53,13 +53,12 @@ public sealed class InFlightByKeyMapTests var party = new InterestedParty(pipe, OriginalTxId: 0x1234); int factoryCalls = 0; - bool ok = map.TryAttachOrCreate( + map.AttachOrCreate( key, party, factory: () => { factoryCalls++; return MakeRequest(party); }, maxParties: 32, out var req, out bool wasNew); - ok.ShouldBeTrue(); wasNew.ShouldBeTrue("a brand-new key must take the create branch"); factoryCalls.ShouldBe(1, "the factory must be called exactly once"); req.ShouldNotBeNull(); @@ -86,15 +85,14 @@ public sealed class InFlightByKeyMapTests var partyB = new InterestedParty(pipeB, OriginalTxId: 0x2222); int factoryCalls = 0; - map.TryAttachOrCreate(key, partyA, + map.AttachOrCreate(key, partyA, factory: () => { factoryCalls++; return MakeRequest(partyA); }, maxParties: 32, out var first, out bool firstWasNew); - bool ok = map.TryAttachOrCreate(key, partyB, + map.AttachOrCreate(key, partyB, factory: () => { factoryCalls++; return MakeRequest(partyB); }, maxParties: 32, out var second, out bool secondWasNew); - ok.ShouldBeTrue(); firstWasNew.ShouldBeTrue(); secondWasNew.ShouldBeFalse("the second attach must coalesce onto the first"); factoryCalls.ShouldBe(1, "the factory must only fire on the create branch"); @@ -126,20 +124,19 @@ public sealed class InFlightByKeyMapTests var partyC = new InterestedParty(pipeC, OriginalTxId: 0xCCCC); // MaxParties = 2 — first attach creates, second appends, third overflows. - map.TryAttachOrCreate(key, partyA, + map.AttachOrCreate(key, partyA, factory: () => MakeRequest(partyA), maxParties: 2, out var first, out _); - map.TryAttachOrCreate(key, partyB, + map.AttachOrCreate(key, partyB, factory: () => MakeRequest(partyB), maxParties: 2, out var second, out _); int factoryCalls = 0; - bool ok = map.TryAttachOrCreate(key, partyC, + map.AttachOrCreate(key, partyC, factory: () => { factoryCalls++; return MakeRequest(partyC); }, maxParties: 2, out var third, out bool thirdWasNew); - ok.ShouldBeTrue(); thirdWasNew.ShouldBeTrue("the third attach must overflow into a fresh entry"); factoryCalls.ShouldBe(1, "the factory must fire to create the overflow entry"); third.ShouldNotBeSameAs(first, "the overflow must be a distinct InFlightRequest"); @@ -167,8 +164,8 @@ public sealed class InFlightByKeyMapTests var partyA = new InterestedParty(pipeA, 1); var partyB = new InterestedParty(pipeB, 2); - map.TryAttachOrCreate(key, partyA, () => MakeRequest(partyA), 32, out _, out _); - map.TryAttachOrCreate(key, partyB, () => MakeRequest(partyB), 32, out _, out _); + map.AttachOrCreate(key, partyA, () => MakeRequest(partyA), 32, out _, out _); + map.AttachOrCreate(key, partyB, () => MakeRequest(partyB), 32, out _, out _); bool removed = map.TryRemove(key, out var req); @@ -228,7 +225,7 @@ public sealed class InFlightByKeyMapTests { if (workCt.IsCancellationRequested) return; var party = new InterestedParty(pipe, (ushort)i); - map.TryAttachOrCreate( + map.AttachOrCreate( key, party, factory: () => MakeRequest(party), maxParties: MaxParties,