wwtools/mbproxy/codereviews/2026-05-16/Multiplexing.md

# Code Review — Backend Connection Layer (Multiplexing + Cache)

**Date:** 2026-05-16
**Branch:** `mbproxy-webui-dashboard` (HEAD `0308490`)
**Scope:** `src/Mbproxy/Proxy/Multiplexing/` (PlcMultiplexer, UpstreamPipe, CorrelationMap, InFlightByKeyMap, InFlightRequest, TxIdAllocator, CoalescingKey, `*LogEvents.cs`), `src/Mbproxy/Proxy/Cache/` (ResponseCache, CacheEntry, CacheKey, CacheInvalidator, CacheLogEvents), `src/Mbproxy/Proxy/MbapFrame.cs`.

## Summary

The connection layer is well-structured and the threading invariants documented in `docs/Architecture/ConnectionModel.md` are largely honoured by the code: single backend writer/reader, per-pipe write loop, `ConcurrentDictionary`-backed correlation map, and a claim-then-dispatch watchdog. The lock-ordering discipline (map lock → no async inside it; connect gate serialising connect vs. teardown) is sound, and the prior reviews' remediations appear intact. However this review found one **wire-protocol correctness bug** that can deliver a stale value to a client across a write (a real data-integrity hazard for an SCADA proxy), one **resource-leak** path, and several **major** correctness gaps around partial-PDU parsing, cache-invalidation arithmetic overflow, and a watchdog/late-attach race window. None of the findings is a deadlock or crash, but the cache-vs-write ordering issue is genuine silent-wrong-value territory and should be treated as the headline item.

**Findings by severity:** Critical: 1 · Major: 6 · Minor: 7

---

## Critical

### C1 — Cache hit can serve a value contradicted by an in-flight write to the same register (read-after-write inversion)

**Files:** `PlcMultiplexer.cs:826-857` (cache lookup), `PlcMultiplexer.cs:674-695` (invalidation on FC06/FC16 *response*).

**What's wrong.** Cache invalidation for a write fires only in `RunBackendReaderAsync` *after the FC06/FC16 response lands from the PLC* (lines 674-695). The cache lookup on a read (lines 826-857) happens with no awareness of writes that are currently *in flight but not yet acked*. Consider this realistic sequence on one PLC, all on the same register range:

1. Client A reads `[100..110)` → cache miss → backend round-trip → response stored in cache (TTL 500 ms).
2. Client B writes register 105 (FC06) → request enqueued, travels to PLC, PLC applies it.
3. Client C reads `[100..110)` → **cache hit** → C receives the *pre-write* value.
4. The FC06 response finally arrives → invalidation runs → cache entry for `[100..110)` dropped.

Between steps 2 and 4 (one full backend round-trip, plus outbound-channel queue depth, plus ECOM scan time — easily tens of ms, far less than a 500 ms TTL) the cache continues to serve the *old* value even though the PLC has already accepted the new one. Client C is told register 105 is the old value *after* the write that changed it was already accepted upstream by the proxy. For a Modbus proxy fronting SCADA, that is a silent wrong value delivered to a client — the exact failure mode the review brief calls Critical.

The doc (`ResponseCache.md` "Write Invalidation") frames invalidation as "a successful FC06/FC16 *response* invalidates" — i.e. the design intentionally invalidates on the response, not the request. That is defensible *only* if the cache also refuses to serve a range while a write to an overlapping range is in flight. It does not: there is no in-flight-write tracking on the read path.

**Impact.** Silent stale value across a write, bounded by one backend round-trip rather than by `CacheTtlMs`. Operationally most visible when a client writes a setpoint then another client (or the same client on a different connection) immediately reads it back and sees the old value. Note coalescing does *not* have this problem — it dies on response — but the cache extends the staleness window past the write.

**Recommendation.** Invalidate (or suppress) on the **request** side, not only the response side. When an FC06/FC16 frame is parsed in `OnUpstreamFrameAsync` (the FC06/FC16 branch around lines 805-817), immediately call `responseCache.Invalidate(unitId, startAddr, qty)` *before* the write is enqueued, and additionally track the overlapping range as "write pending" so reads that arrive before the write response lands also miss. The simplest correct fix: invalidate on request enqueue *and* keep the existing response-side invalidation as a backstop. The minor cost (a write may evict an entry that the write later turns out to fail with an exception) is acceptable — a failed write just causes a cache miss and a fresh read, which is always safe; serving a stale value is not.

---

## Major

### M1 — `EnsureBackendConnectedAsync` leaks the backend socket if `_disposed` flips during connect

**File:** `PlcMultiplexer.cs:287-358`.

**What's wrong.** A caller can enter `EnsureBackendConnectedAsync`, pass the `_disposed` check at line 289, acquire `_connectGate`, and run a multi-second Polly connect (lines 310-324). If `DisposeAsync` runs concurrently, it sets `_disposed = true`, cancels `_disposeCts`, and calls `TearDownBackendAsync` — which acquires the gate (or times out after 2 s and proceeds gate-less), and at that moment `_backendSocket` is still `null` because the connecting task has not yet reached line 341. `TearDownBackendAsync` sees `oldSocket is null && oldCts is null` and returns without closing anything. The connecting task then completes its `ConnectAsync`, takes `_backendLock`, and assigns a **fully live `Socket`** plus three running tasks (`_backendWriterTask`/`ReaderTask`/`HeartbeatTask`) into the now-disposed multiplexer. Nothing ever disposes that socket or joins those tasks; `_disposeCts` is already disposed so `CancellationTokenSource.CreateLinkedTokenSource(_disposeCts.Token)` at line 338 throws `ObjectDisposedException`, leaving `backend` connected and undisposed.

**Impact.** Leaked TCP socket to the PLC (one of the ECOM's 4 client slots held forever) plus three orphaned background tasks per occurrence. Occurs on a hot-reload PLC-remove or service stop that races a cold-start connect — not common, but it happens under exactly the churn the supervisor is designed to handle.

**Recommendation.** After a successful connect, re-check `_disposed` (and `_disposeCts.IsCancellationRequested`) *under `_backendLock`* before publishing the socket; if disposal won the race, dispose `backend` and return `false`. Wrap the `CreateLinkedTokenSource` in the same guarded block so a disposed `_disposeCts` cannot throw past the socket assignment.

### M2 — Backend reader drops a frame whose PDU body length disagrees with the PLC, with no resync

**File:** `PlcMultiplexer.cs:540-574`, `UpstreamPipe.cs:111-163`.

**What's wrong.** Both `FillAsync` loops trust the MBAP `Length` field absolutely. If the backend (or a middlebox) ever produces a frame whose `Length` field is wrong — too short — the reader consumes `length-1` body bytes, then loops and reinterprets the *next* bytes (which are really still part of the previous PDU) as a fresh 7-byte MBAP header. From that point every subsequent frame on the socket is mis-framed: `proxyTxId` is garbage, `CorrelationMap.TryRemove` misses, frames are silently dropped (line 580-584), and every real in-flight request leaks until the watchdog times it out with 0x0B. The socket never faults, so no cascade fires; the PLC effectively goes dark for `BackendRequestTimeoutMs` repeatedly. The only oversized-frame guard (line 561) catches `length-1 > 253` but not a *too small* / mis-aligned length.

**Impact.** A single corrupt length field on the wire desynchronises the backend stream indefinitely. Recovery depends entirely on the backend eventually closing the socket. Realistic on flaky industrial networks; the brief explicitly lists "middlebox drops a packet" as a covered scenario.

**Recommendation.** This is hard to fully solve without a length-validation/resync strategy, but at minimum: when `RunBackendReaderAsync` removes a correlation entry it can sanity-check the response PDU length against the request (e.g. an FC03 response body should be `2 + 2*qty`); a mismatch is strong evidence of desync and should `break` to force a teardown+cascade rather than silently fan out a wrong-sized payload. At a minimum, document that the reader assumes a well-framed backend and add a desync-suspected counter when `TryRemove` misses repeatedly.

### M3 — FC03/FC04 cache hit fans out a payload whose register count is not validated against the request

**Files:** `PlcMultiplexer.cs:826-857` (`BuildCacheHitFrame`), `PlcMultiplexer.cs:1356-1370`.

**What's wrong.** `CacheKey` includes `Qty`, so a hit only occurs for an identical `(unit,fc,start,qty)` — good. But `BuildCacheHitFrame` splices `cached.PduBytes` onto a fresh MBAP header and sets `Length = 1 + pduLen` with no upper-bound check. The stored PDU came from the backend reader (line 645-646, snapshot of `pduBodyLen` bytes which *is* bounded to ≤253 at line 561), so today it cannot exceed spec. However nothing asserts the cached PDU's declared byte-count actually matches `2*qty`. If a backend ever returned a short/long FC03 body that still passed the ≤253 check, that malformed payload is now cached and replayed to *every* future hit for the lifetime of the TTL — the cache amplifies one bad backend response into many bad client responses. Coalescing has the same single-response blast radius but only for the concurrently-attached parties; the cache widens it across time.

**Impact.** Lower likelihood than C1/M2 but the same class of harm — a wrong-shaped frame delivered to clients, amplified by caching.

**Recommendation.** When storing an FC03/FC04 response (line 641-667), validate the PDU body: `frame[HeaderSize+1]` (byte count) must equal `2*qty` and `pduBodyLen` must equal `2 + 2*qty`. Skip the cache store (and ideally skip the fan-out / force teardown) on mismatch. This also strengthens M2's resync story.

### M4 — `CacheInvalidator.FindOverlapping` and the FC16 parse can integer-overflow the half-open upper bound

**Files:** `CacheInvalidator.cs:38,46`, `PlcMultiplexer.cs:812-817`.

**What's wrong.** `writeEnd = writeStart + writeQty` and `keyEnd = key.StartAddress + key.Qty` are computed as `int` from two `ushort`s — that part is fine (max 0x1FFFE, no overflow). But the FC16 parse at `PlcMultiplexer.cs:815-816` reads `qty` straight from the wire with no clamp: a malicious or buggy client can send FC16 with `qty = 0xFFFF`. That `qty` is then used both as the `InFlightRequest.Qty` and, on the FC06/FC16 response, passed to `postCache.Invalidate(unitId, startAddress, qty)`. The proxy never validates that `qty` is within the DL260's FC16 cap of 100 (per `dl205.md` / CLAUDE.md) — the comment says "the PLC enforces the cap with exception 03", which is true for the *write itself*, but the **invalidation still runs with the bogus 65535-register span** because invalidation is gated only on `isException == false`. If the PLC accepts a smaller write and returns success, an attacker-supplied wide `qty` cannot reach here — but an FC16 request whose declared `qty` does not match its byte-count payload, accepted by a lax backend, would invalidate a 64K-register span and flush most of the cache. More concretely: the FC16 *request* `qty` and the *response* `qty` are assumed identical (the response echoes start+qty), but the code uses the **request-side** `inFlight.Qty` for invalidation (line 687) — so a request that lies about `qty` drives invalidation regardless of what the PLC actually wrote.

**Impact.** Cache-flush amplification / mild DoS on the cache from a malformed FC16. Not memory-unsafe (the `int` math does not overflow), but a contract violation: invalidation should reflect what was *written*, and it currently reflects what the client *claimed*.

**Recommendation.** Validate FC16 framing in `OnUpstreamFrameAsync`: require `qty` in `1..123` (or the DL260 cap of 100) and require `frame.Length >= pduOffset + 6 + 2*qty` and `byteCount == 2*qty`; reject (exception 03) otherwise rather than forwarding. Also prefer driving invalidation from the FC16 *response* PDU's echoed start/qty rather than the request's.

### M5 — Watchdog can miss a late-attached coalescing party, hanging that upstream until *its* socket times out

**Files:** `PlcMultiplexer.cs:1126-1161`, `InFlightByKeyMap.cs:59-83`.

**What's wrong.** The watchdog snapshots stale entries from `CorrelationMap` (line 1123), then for each one does `TryRemove` from `CorrelationMap` (line 1131) and only *afterwards* removes the `CoalescingKey` from `InFlightByKeyMap` (line 1160). Between the `CorrelationMap.TryRemove` and the `_inFlightByKey.TryRemove` there is a window where a new FC03/FC04 request can call `AttachOrCreate`, find the still-present coalescing entry, and append itself to `req.InterestedParties`. The watchdog then walks `req.InterestedParties` at line 1165 — but it captured `req` from the `CorrelationMap.TryRemove` out-param, and `InterestedParties` is the *same mutable `List<InterestedParty>`*, so a party appended after the `foreach` started its enumeration would throw `InvalidOperationException` ("collection modified") — caught by the outer `catch (Exception)` at line 1195, which logs and **exits the watchdog loop entirely**. A party appended *before* the `foreach` but after the `CorrelationMap.TryRemove` is delivered the 0x0B (fine), but a party that attached and whose request was *never enqueued to the backend* (the coalescing entry is now orphaned — no proxy TxId of its own) gets nothing if the enumeration already passed it.

The doc (`ReadCoalescing.md` "Multi-writer multi-reader safety") claims the watchdog "removes the coalescing key before it walks `req.InterestedParties`". The code does the opposite ordering: `CorrelationMap` removal first, then the walk at 1165, then `_inFlightByKey` removal at 1160 is actually *before* the walk — re-reading: line 1157-1161 (`_inFlightByKey.TryRemove`) runs before line 1165 (`foreach`). So the key *is* removed before the walk. Good. **But** the `CoalescingKey` removal at 1160 only happens for `req.Fc is 0x03 or 0x04`; a non-coalescing FC still has no key. The real residual hole: a late attach in the window between `CorrelationMap.TryRemove` (1131) and `_inFlightByKey.TryRemove` (1160) appends to the list, and the `foreach` at 1165 then enumerates a list that *was mutated after `TryRemove`*. If the append lands during the `foreach`, `InvalidOperationException` kills the watchdog (caught at 1195 → loop exits → **no more timeouts ever fire for this PLC**).

**Impact.** A rare race can permanently disable the timeout watchdog for one PLC, after which any lost backend response leaks its correlation entry forever and hangs upstream clients indefinitely — defeating the entire reason the watchdog exists.

**Recommendation.** Remove the `CoalescingKey` from `_inFlightByKey` *before* removing from `CorrelationMap`, mirroring the backend reader's ordering (reader does `CorrelationMap.TryRemove` then `_inFlightByKey.TryRemove` — actually the reader has the same ordering; see N-note). The robust fix: in `InFlightByKeyMap`, hand fan-out callers a *frozen copy* of the party list at removal time, or snapshot `req.InterestedParties.ToArray()` immediately after the claim and iterate the copy. Apply the same snapshot in `RunBackendReaderAsync`'s fan-out loop (line 710) for symmetry.

### M6 — `RunBackendReaderAsync` cascade-on-EOF/fault is fire-and-forget; an exception inside `TearDownBackendAsync` is unobserved

**Files:** `PlcMultiplexer.cs:753-766`, `538-537`, `RunBackendHeartbeat/Watchdog` similar.

**What's wrong.** Reader EOF (line 756), reader fault (765), and writer fault (536) all start `TearDownBackendAsync(...)` with `_ = Task.Run`-style fire-and-forget (just `_ = TearDownBackendAsync(...)`). `TearDownBackendAsync` is `async Task`; if it throws *synchronously before its first await* — or after, on a path the `try/finally` doesn't cover — the exception lands on an unobserved `Task`. The code guards the common case (`if (!_disposeCts.IsCancellationRequested)`) to avoid the disposed-`_connectGate` race, but `_connectGate.WaitAsync` can still throw `ObjectDisposedException` if disposal flips *between* the check and the call — that path is handled inside `TearDownBackendAsync` (line 398-403, returns). The residual risk is narrower than M1 but the pattern (unobserved fire-and-forget `async Task` from three call sites) is fragile: any future exception added to `TearDownBackendAsync`'s pre-await section becomes a process-level `UnobservedTaskException`.

**Impact.** Latent; today probably benign. Worth hardening because all three of the layer's failure-detection paths funnel through it.

**Recommendation.** Wrap the fire-and-forget in a helper that attaches a continuation logging any fault: `_ = TearDownBackendAsync(...).ContinueWith(t => _logger.LogError(t.Exception, ...), TaskContinuationOptions.OnlyOnFaulted)`. Or make the call sites `await` it where the calling context allows (the reader/writer tasks can `await` since they are about to exit anyway).

---

## Minor

### N1 — `UpstreamPipe.RunReadLoopAsync` permits a 7-byte zero-body frame but `OnUpstreamFrameAsync` cannot route it

**Files:** `UpstreamPipe.cs:133-141`, `PlcMultiplexer.cs:775-797`.

A `Length < 1` frame is forwarded as a header-only `byte[7]` (line 136-138). `OnUpstreamFrameAsync` then reads `fcByte = frame.Length > pduOffset ? ... : 0` → `fcByte = 0`, falls through every FC branch, allocates a proxy TxId, and forwards a 7-byte frame to the backend. A 7-byte MBAP frame with `Length=0` is itself malformed Modbus (`Length` must be ≥2: UnitId+FC). The proxy should reject it rather than allocate a TxId and forward garbage. Recommend: in the read loop, treat `length < 2` as a protocol error and close the pipe (consistent with the oversized-frame handling).

### N2 — Backend reader's `_inFlightByKey.TryRemove` runs *after* `CorrelationMap.TryRemove` but the doc invariant wants it before fan-out only

**File:** `PlcMultiplexer.cs:580-607`.

The ordering is correct for the stated invariant (key removed at 602-607, fan-out at 710), but the `CoalescingKey` is reconstructed from `inFlight.UnitId/Fc/StartAddress/Qty` (line 604-605) — if `inFlight` were ever a heartbeat with `Fc=0x03` this would spuriously probe the map; it is guarded by the `IsHeartbeat` early-`continue` at 595-596, so this is safe today. Worth a one-line comment that the FC03/FC04 branch is heartbeat-free by construction, so a future refactor doesn't reorder the `IsHeartbeat` check below it.

### N3 — `TxIdAllocator.Release` silently no-ops a double-release, masking the documented `TearDownBackendAsync` race

**Files:** `TxIdAllocator.cs:119-129`, `PlcMultiplexer.cs:377-385`.

The "KNOWN RACE" comment block at `PlcMultiplexer.cs:377-385` describes a double-release that frees a legitimately in-flight slot. `Release` cannot detect this — it just checks `_inUse[id]`. Consider having the allocator track a generation/epoch per slot, or at minimum increment a `doubleReleaseSuspected` counter when `Release` is called on an already-free slot, so the (admittedly very rare) silent-request-drop the comment accepts becomes observable in production rather than invisible.

### N4 — `ResponseCache` LRU eviction is O(n) per insert and the doc's 1000-entry assumption is unenforced at the call path

**File:** `ResponseCache.cs:206-230`.

`EvictLeastRecentlyUsed` is a full linear scan, acceptable at the documented 1000-entry cap. But `_maxEntries` comes from `Cache.MaxEntriesPerPlc` which is operator-settable with no documented ceiling in this file; a fat-fingered `MaxEntriesPerPlc = 1_000_000` turns every cache insert into a million-element scan under the lock, stalling the backend reader for every cache-miss store. Recommend either documenting/validating a hard ceiling or switching to an intrusive LRU (linked-list + dict) if large caps must be supported.

### N5 — `SnapshotOlderThan` allocates a `List` every watchdog tick even when nothing is stale

**File:** `CorrelationMap.cs:72-81`, `PlcMultiplexer.cs:1123-1124`.

The watchdog ticks every `BackendRequestTimeoutMs/4` (≥100 ms) and always allocates a fresh `List`, then checks `.Count == 0`. On an idle fleet of 54 PLCs that is 540 throwaway lists/sec. Trivial, but `SnapshotOlderThan` could return `Array.Empty` when the map is empty, or the watchdog could skip the call when `_correlation.Count == 0`.

### N6 — `CacheEntry.CapturedTags` replay on a cache hit re-stamps observations "now" for every hit, including hits served during a backend outage

**File:** `PlcMultiplexer.cs:844-850`.

When the cache serves a hit during a `recovering` backend, the debug view's `capture.Record(...)` is called with `CaptureDirection.Read` and the current time, making the detail page show a "fresh" read of a tag that has not actually been read from the PLC for up to `CacheTtlMs`. This is arguably misleading for an operator debugging during an outage — the value is cache-aged, not live. Consider tagging replayed observations as cache-served, or stamping them with the entry's `CachedAtUtc` rather than now.

### N7 — Doc drift: `ConnectionModel.md` says the cascade closes "every attached pipe" but the inline comment at the cascade still references the superseded "in flight" wording

**File:** `PlcMultiplexer.cs:448-450`.

The comment reads "Close every attached pipe that had a request in flight; the others will simply re-issue" — directly contradicted by the next line ("Per docs/.../ConnectionModel.md, ALL attached upstreams cascade") and by `upstreamCount = _pipes.Count`. The code is correct (all pipes cascade); the first comment sentence is stale and should be deleted to avoid confusing a future reader into thinking idle pipes survive.

---

## Notes on things checked and found correct

- **TxId wraparound** (`TxIdAllocator.cs`): the forward scan with `_inFlightCount < SlotCount` guard correctly terminates; `_wrapCount` increments on `0xFFFF→0x0000` cursor roll. No off-by-one. Reuse-distance maximisation works as documented.
- **Claim-then-dispatch in the watchdog** (`PlcMultiplexer.cs:1131`): the `TryRemove` race against a real response is correctly handled — whoever wins `TryRemove` owns dispatch; the loser skips. No double-delivery of a real response + 0x0B for the *non-coalesced* case.
- **MBAP TxId preservation**: proxy TxId is written at request enqueue (lines 1014-1015 / 1080-1081) and the original is restored per-party on the response (lines 736-737); heartbeat and cache-hit and exception frames all carry the correct TxId. Verified correct.
- **Single-party fan-out buffer reuse** (line 732-734): the `Count == 1` fast path reuses the buffer; multi-party clones per party. No aliasing bug.
- **Connect gate vs. teardown**: `_connectGate` correctly serialises `EnsureBackendConnectedAsync` against `TearDownBackendAsync`; the bounded 2 s teardown wait and its documented best-effort fallback are reasonable (the residual race is C-noted at N3, not a new finding).
- **`InFlightByKeyMap` lock discipline**: no async work happens under the lock; the factory is invoked under the lock but does only synchronous allocation/`TryAdd`. No lock-ordering inversion against `TxIdAllocator`'s lock (allocator lock is always taken *inside* the map lock, never the reverse).
- **Cache `recovering`-state invalidation skip**: the structural argument in the inline comment (lines 676-685) holds — no backend reader exists in `recovering`, so no FC06/FC16 response can drive an invalidation. Correct as written (but see C1, which is a *different* and real problem on the read side).