mbproxy: Wave 3 cleanups, docs, and test gaps from 2026-05-14 review
Closes the Wave 3 (cleanup) tier of codereviews/2026-05-14/RemediationPlan.md.
Tests: 378 pass / 0 fail (baseline 370 + 8 new W3 regression tests).
Code cleanups:
* PlcMultiplexer: removed dead `elapsedMs` calculation (the actual EWMA
conversion uses Stopwatch ticks two lines below).
* UpstreamPipe.FillAsync: dropped the meaningless `firstRead && remaining
== count ? false : false` ternary; both branches were `false`.
* InFlightByKeyMap.TryAttachOrCreate (always returned `true`) renamed to
`AttachOrCreate` and made `void`. Test sites updated to drop the dead
`bool ok = ...; ok.ShouldBeTrue();` assertions.
* BcdCodec.HasBadNibble promoted from private to internal; the duplicate
copy in BcdPduPipeline removed and the call sites updated to
`BcdCodec.HasBadNibble`.
* PlcMultiplexer watchdog comment fixed: said "1-second floor", code uses
100 ms. Now both agree.
* StatusSnapshotBuilder: simplified the unreachable
`RemoteEp?.ToString() ?? RemoteEp?.Address.ToString() ?? "?"` to
`RemoteEp?.ToString() ?? "?"`.
* Mbproxy.csproj: stale "deferred" Polly comment replaced with a real
description of where Polly is used (BackendConnect + ListenerRecovery).
Doc updates:
* README: added a callout about the unconventional 32-bit BCD wire format
("two base-10000 digits in CDAB", not standard binary CDAB Int32) so
integrators using off-the-shelf clients learn about the silent-corruption
hazard before configuring writes.
* docs/design.md: clarified `cacheMissCount` and `coalescedMissCount`
semantics — "miss" means "did not find a fresh entry / did not coalesce",
NOT "produced a backend round-trip". Operators wanting actual backend
traffic should compute `miss − coalescedHit − exception04`.
* docs/Architecture/ResponseCache.md: documented the structural
"skip invalidation while recovering" gating (no backend reader during
recovery → no FC06/FC16 response → no invalidation).
* docs/Operations/Configuration.md: noted that the Event Log sink is the
custom EventLogBridge, not Serilog.Sinks.EventLog (W2.23 cached check).
* docs/plan/README.md: added a Phase 12 row pointing at the remediation
plan and linking out to codereviews/2026-05-14/.
Test additions (W3 high-value gaps):
* BcdPduPipelineTests:
- FC16_WriteStartsOnHighWord_Of32BitPair_PassesThroughRaw_WithPartialWarning
(symmetric inverse of the existing low-side partial-overlap test).
- FC03_Mixed_16Bit_32Bit_AndNonBcd_InOneRead_OnlyConfiguredSlotsRewritten
(mixed-slot routing in a single FC03 read).
- FC16_Response_PassesThroughUnchanged_RegardlessOfTagMap (FC16 response
carries no register data; rewriter must pass through).
* AdminEndpointTests:
- NonGetMethod_AgainstAdminRoutes_Returns405 (Theory: POST/PUT/DELETE/
PATCH against `/` and `/status.json` must return 405; guards against
an accidental MapPost being added later).
* HotReloadE2ETests:
- E2E_TagListReload_OnCacheablePlc_EmitsCacheFlushedEvent (validates the
W2.8 cache.flushed wiring end-to-end via the real FileSystemWatcher
reload path).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
@@ -66,7 +66,11 @@ internal sealed class StatusSnapshotBuilder
|
||||
var activeUpstreams = supervisor?.ActiveUpstreams ?? Array.Empty<UpstreamPipe>();
|
||||
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();
|
||||
|
||||
@@ -97,15 +97,18 @@ internal static class BcdCodec
|
||||
return hiVal * 10_000 + loVal;
|
||||
}
|
||||
|
||||
// ── Private helpers ─────────────────────────────────────────────────────
|
||||
// ── Helpers ─────────────────────────────────────────────────────────────
|
||||
|
||||
/// <summary>Returns true if any nibble in <paramref name="raw"/> is >= 0xA.</summary>
|
||||
private static bool HasBadNibble(ushort raw)
|
||||
/// <summary>
|
||||
/// Returns true if any nibble in <paramref name="raw"/> is >= 0xA (i.e. a non-BCD
|
||||
/// digit). Internal so <see cref="Mbproxy.Proxy.BcdPduPipeline"/> can call it from
|
||||
/// the response-rewrite path's per-word check without re-implementing the same logic.
|
||||
/// </summary>
|
||||
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;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -39,7 +39,8 @@
|
||||
<PackageReference Include="Serilog.Settings.Configuration" Version="10.0.0" />
|
||||
<PackageReference Include="Serilog.Sinks.Console" Version="6.1.1" />
|
||||
<PackageReference Include="Serilog.Sinks.File" Version="7.0.0" />
|
||||
<!-- Referenced now so phase 04/05 don't need to touch this csproj; usage is deferred -->
|
||||
<!-- Polly: backend-connect retry pipeline (PolicyFactory.BuildBackendConnect) and
|
||||
listener-recovery pipeline (PolicyFactory.BuildListenerRecovery). -->
|
||||
<PackageReference Include="Polly" Version="8.6.6" />
|
||||
</ItemGroup>
|
||||
|
||||
|
||||
@@ -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 ──────────────────────────────────────────────────────────────
|
||||
|
||||
/// <summary>Returns true if any nibble of <paramref name="raw"/> is >= 0xA.</summary>
|
||||
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).
|
||||
}
|
||||
|
||||
@@ -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).</para>
|
||||
///
|
||||
/// <para>Returns <c>true</c> always (the bool return matches the phase doc's signature;
|
||||
/// future evolution could introduce a refusal path).</para>
|
||||
/// <para>Phase 12 (W3 cleanup) — was previously declared as <c>bool TryAttachOrCreate</c>
|
||||
/// but always returned <c>true</c>. The bool was dead; the result is in the
|
||||
/// <paramref name="wasNew"/> out parameter.</para>
|
||||
/// </summary>
|
||||
public bool TryAttachOrCreate(
|
||||
public void AttachOrCreate(
|
||||
CoalescingKey key,
|
||||
InterestedParty party,
|
||||
Func<InFlightRequest> 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;
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user