# Re-Review After Remediation — 2026-05-14

Re-review of the codebase after the six-commit remediation of the original 2026-05-14 review (Wave 1 → `ce32c5c`, Wave 2 → `e66b17f`, Wave 3 → `7ead358`, the easy 5 → `2545237`, the race-hard 5 → `53f842a`). Conducted via three parallel area-focused passes. **Eyes on what the fixes themselves introduced**, not what the original review already found.

**Scope:** every src/ and tests/ change in `53a7111..HEAD` (37 files, ~+2000/−700 lines).

## Status

> **All actionable findings resolved across two re-review passes.** Wave 4 (`7a43595`) closed NC1 + NM1 + NM2 + NM5 + Nm1 + T2. Wave 4-followup (`9251c56`) closed NM3 + NM4 + Nm6 + Nm7 + T3 + T4. A third focused pass surfaced one more major (W5/M1) and two cosmetics (W5/m1, W5/m2); Wave 5 (this commit) resolved M1 + m2 and documented m1 as accepted best-effort.
>
> **Final test count:** 387 pass / 0 fail.

## Headline

The remediation was structurally sound. The re-review found:

- **1 critical finding** — the W1.5 drain loop inherited the very `ShutdownCoordinator` bug it was meant to replace. **Resolved** by Wave 4 (`7a43595`) snapshotting the in-flight count BEFORE supervisor stop and deleting the theatrical post-stop loop.
- **5 major findings** clustered in W1.4 cascade gating and W1.1 + W1.5 lifecycle ordering. **All resolved** by Wave 4 + Wave 4-followup.
- **8 minor findings** + **4 test-discipline findings**. Most resolved; the rest accepted with documented rationale.

## Resolution table

| ID | Severity | Finding | Status | Commit |
|----|----------|---------|--------|--------|
| **NC1** | Critical | `ProxyWorker.StopAsync` drain loop structurally always-zero — inherited the original `ShutdownCoordinator` bug | ✅ **Resolved** | `7a43595` |
| **NM1** | Major | `TearDownBackendAsync._connectGate.WaitAsync()` uncancellable — disposal can be blocked indefinitely | ✅ **Resolved** | `7a43595` |
| **NM2** | Major | `ReplaceContext` writes `_ctx` and re-registers cache stats provider non-atomically — snapshot can read a disposed cache | ✅ **Resolved** | `7a43595` |
| **NM3** | Major | `_supervisorCts` leaks across `StartAsync` re-entry despite W2.16 guard | ✅ **Resolved** | (W4-followup) |
| **NM4** | Major | W2.15 TCS never re-armed — supervisor effectively single-shot | ✅ **Resolved** | (W4-followup) |
| **NM5** | Major | Self-cascade swallows `ObjectDisposedException` from `_connectGate` after disposal | ✅ **Resolved** | `7a43595` |
| **Nm1** | Minor | Saturation cleanup uses `await SendResponseAsync` (blocking) per attached pipe | ✅ **Resolved** | `7a43595` |
| **Nm2** | Minor | W1.2 increments `CoalescedHit` for late attachers that ultimately receive exception 04 | ⚪ **Accepted** | (doc) |
| **Nm3** | Minor | Both supervisor-stop and drain shared `gracefulMs` budget | ✅ **Resolved** | `7a43595` (drain deleted) |
| **Nm4** | Minor | `finally` preserves `_lastBindError` after clean cancellation | ⚪ **Accepted** | (by design) |
| **Nm5** | Minor | `EventLogBridge` no startup log of armed state | ⚪ **Accepted** | (low value) |
| **Nm6** | Minor | `_admin` lazy resolution returns `null` silently if registration absent | ✅ **Resolved** | (W4-followup) |
| **Nm7** | Minor | `AdminEndpointHost.DisposeAsync` no `_disposed` guard | ✅ **Resolved** | (W4-followup) |
| **Nm8** | Minor | `TearDownBackendAsync` cosmetic log-noise on queued cascades | ⚪ **Accepted** | (cosmetic) |
| **T1** | Test | Reflection coupling on private field names | ⚪ **Accepted** | (commented in code) |
| **T2** | Test | `WatchdogVsResponse_Race` seeded `Random` cross-runtime fragility | ✅ **Resolved** | `7a43595` |
| **T3** | Test | `RemoveInheritedAppsettings` only fires on Build, not Publish | ✅ **Resolved** | (W4-followup) |
| **T4** | Test | Stale `TryAttachOrCreate_*_ReturnsTrue_*` test method names after W3 dropped the bool | ✅ **Resolved** | (W4-followup) |

**Resolved: 13/18. Accepted: 5/18.**

## Third pass — final findings (Wave 5)

A third focused review pass on the post-W4-followup state turned up these additional items:

| ID | Severity | Finding | Status | Commit |
|----|----------|---------|--------|--------|
| **W5/M1** | Major | `AdminEndpointHost` `OnChange` callback can resurrect a Kestrel app after `StopAsync` returned (no `_disposed` check inside the fire-and-forget Task.Run lambda) | ✅ **Resolved** | (W5) |
| **W5/m1** | Minor | `TearDownBackendAsync` gate-not-held path: a concurrent freshly-allocated TxId can collide with one being released by the channel drain → silent request drop. Probability very low (gate timeout AND new accept AND TxId collision in 65,536-slot space). | ⚪ **Accepted** | (W5 — inline doc comment in `PlcMultiplexer.cs`) |
| **W5/m2** | Minor | `inFlightAtCancel` was computed AFTER `base.StopAsync` — narrower window than the field name promises | ✅ **Resolved** | (W5) |
| **W5/m3** | Cosmetic | `CountInFlight` allocates a 35-field `CounterSnapshot` record per supervisor on shutdown | ⚪ **Accepted** (skip) | — |

**W5/M1 fix detail.** Added `if (_disposed) return;` at the top of the `OnChange` lambda AND inside the queued `Task.Run`, plus `try/catch (ObjectDisposedException)` around `_lock.WaitAsync` and `_lock.Release()` so a hot-reload of `AdminPort` during shutdown can no longer resurrect a fresh Kestrel WebApplication on the new port after the host considered admin shut down.

**W5/m2 fix detail.** Moved `int inFlightAtCancel = CountInFlight();` to BEFORE `await base.StopAsync(cancellationToken)`. Now the count actually reflects "in-flight at the moment the host signalled stop" — not "in-flight at the moment we got around to computing it after the cancel propagated."

**W5/m1 acceptance.** Documented inline at `PlcMultiplexer.cs:TearDownBackendAsync` near the `gateHeld` flag declaration. The race requires three coincidences (gate-timeout + new accept landing during cascade + TxId collision); the only consequence is one dropped request that the client retries on its next attempt.

**W5/m3 skip.** Trivial per-PLC allocation (~5 KB on a 54-PLC fleet, called once per shutdown). Optimising it would require exposing a single-field accessor on `ProxyCounters`; not worth the surface change.

---

## Resolved findings — what landed

### NC1 — `ProxyWorker.StopAsync` drain loop is structurally always-zero
**Resolution:** `7a43595` snapshots `inFlightAtCancel = CountInFlight()` BEFORE calling `supervisor.StopAsync(...)`. The post-stop drain loop and `drainCts` are deleted entirely. The supervisor stop IS the drain — there's nothing to wait for that wouldn't be killed by the stop itself. `mbproxy.shutdown.complete` now reports a meaningful "requests dropped by stop" count.

### NM1 — `TearDownBackendAsync._connectGate.WaitAsync()` uncancellable
**Resolution:** `7a43595` bounds the wait with a 2-second teardown CTS. On timeout the body proceeds best-effort without the gate; `gateHeld` flag tracks whether we acquired it so the `finally` only releases when appropriate. `ObjectDisposedException` from a disposed semaphore short-circuits to a clean return.

### NM2 — `ReplaceContext` non-atomic ctx + provider swap
**Resolution:** `7a43595` swapped the order: provider FIRST, then `_ctx`. Snapshots in the swap window now read either (old, old) or (new, new) — never (old-after-disposed).

### NM3 — `_supervisorCts` leaks across `StartAsync` re-entry
**Resolution:** W4-followup. `StartAsync` now does `try { _supervisorCts.Dispose(); } catch (ObjectDisposedException) { }` before reassigning, with the catch covering the very-first-Start case where the field-init CTS is still fresh.

### NM4 — W2.15 TCS never re-armed (supervisor single-shot)
**Resolution:** W4-followup. `_firstAttemptCompleted` is now non-readonly and re-created in `StartAsync` after the W2.16 guard. A re-Started supervisor's `WaitForInitialBindAttemptAsync` no longer observes the previous run's signal.

### NM5 — Self-cascade `ObjectDisposedException` after dispose
**Resolution:** `7a43595` gated the writer + reader fault-path `_ = TearDownBackendAsync(...)` calls behind `if (!_disposeCts.IsCancellationRequested)`. DisposeAsync runs an explicit teardown anyway, so the fire-and-forget path was redundant *and* exception-throwing.

### Nm1 — Saturation cleanup uses `await SendResponseAsync`
**Resolution:** `7a43595` replaced the per-party `await SendResponseAsync` with `TrySendResponse` and `IncrementResponseDropForFullUpstream` on drop. Same doctrine as W1.3.

### Nm3 — `gracefulMs` used twice
**Resolution:** Implicit — Wave 4's NC1 fix deleted the drain loop, so the budget is now used exactly once (supervisor stop). Worst-case shutdown is `gracefulMs + 2 s admin`.

### Nm6 — `_admin` lazy resolution silent null
**Resolution:** W4-followup. `ProxyWorker.ExecuteAsync` now logs a Warning when `GetService<AdminEndpointHost>()` returns null, surfacing a botched composition without blocking startup. Previous IHostedService registration would have hard-errored in DI; this preserves the loud-failure intent without forcing callers to register admin in unit-test hosts.

### Nm7 — `AdminEndpointHost.DisposeAsync` no `_disposed` guard
**Resolution:** W4-followup. Added a `volatile bool _disposed` flag and a guard at the top of `DisposeAsync`; symmetry with `PlcMultiplexer`.

### T2 — `WatchdogVsResponse_Race` seeded `Random` fragility
**Resolution:** `7a43595` replaced `new Random(12345) → rng.Next(350, 450)` with a counter-based alternation `(n & 1) == 1 ? 350 : 450`. Guaranteed 15 fast + 15 slow across 30 iterations regardless of runtime.

### T3 — `RemoveInheritedAppsettings` only on Build
**Resolution:** W4-followup. `AfterTargets="Build;Publish"` and a second `Delete` against `$(PublishDir)appsettings.json` (guarded by `Condition="'$(PublishDir)' != ''"` so a plain Build doesn't trip it).

### T4 — Stale `TryAttachOrCreate_*` test names
**Resolution:** W4-followup. Renamed three test methods to drop `Try` and `_ReturnsTrue_` to match the W3 `AttachOrCreate` (void-returning) signature.

---

## Accepted findings — what wasn't fixed and why

### Nm2 — `CoalescedHit` increments for late attachers that get exception 04
**Why accepted:** The counter is correct per the design contract (`coalescedHit + coalescedMiss = total FC03/FC04 requests`). A late attacher *did* coalesce onto a stub; the fact that the stub later delivered an exception instead of a real response is orthogonal to the coalesce-counting semantics. Decrementing on saturation cleanup would break the parity invariant that operator dashboards rely on. Documented inline at `PlcMultiplexer.cs:809-812` (W2.4 doc clarification covers this).

### Nm4 — `_lastBindError` preserved after clean cancellation
**Why accepted:** Intentional. The field's contract is "last fault while running" — useful for operators triaging *why* a supervisor exited (was it a bind error vs a clean stop?). Clearing it on clean exit would lose forensic information. The `finally` block at `PlcListenerSupervisor.cs:435-436` already documents this with a comment.

### Nm5 — `EventLogBridge` no startup log of armed state
**Why accepted:** The bridge is a Serilog sink and doesn't have an injectable logger (would require restructuring to use `Log.ForContext` or similar). The class doc at `EventLogBridge.cs:18-20` already tells operators "must be created by `install.ps1` before the service starts". The W2.23 caching means a missing source is silent, but the architectural alternative (log from the bridge constructor using `Log.Logger`) creates a startup-ordering dependency on Serilog being fully wired before any sink construction. Low value vs the surface change.

### Nm8 — `TearDownBackendAsync` log-noise on queued cascades
**Why accepted:** Cosmetic. Each cascade logs its own `BackendDisconnected` event with its own counts; queued cascades on the gate would fire mostly-zero events. Log filtering is the operator's tool here.

### T1 — Reflection on private field names in race tests
**Why accepted:** The two reflection sites (`DrainAllocator` in `PlcMultiplexerTests`, the runtime-fault test in `SupervisorTests`) are the only externally-impossible-to-hit primitives that prove the saturation/runtime-fault contracts. The alternatives are (a) introducing test-only `internal` accessors that pollute production code, (b) exposing fields via `[InternalsVisibleTo]` (which Mbproxy.csproj already does for the test project — but the *fields* themselves are private, not internal), or (c) skipping the tests. The reflection coupling is documented as a maintenance trap in the test code. A future rename refactor breaks at run-time, not compile-time — but the tests' xmldoc explicitly warns about this.

---

## Verified clean (sampled, not exhaustive)

The original re-review listed the following as verified clean by inspection during the post-Wave-3 review pass; nothing in Wave 4 / W4-followup invalidated these:

- **W2.3 ConcurrentDictionary migration** on `_supervisors` — all mutations atomic; status-page enumeration lock-free; Restart's "remove + add" two-step is per-key (parallel keys disjoint by name).
- **W2.1 coalescingAccessor propagation** — both Add and Restart paths receive it; Reseat correctly does not (same supervisor, same multiplexer, same accessor).
- **W2.13 OOR check** — multiplication is bounded by the guard; even worst-case `9999 * 10_000 + 9999 = 99_989_999` fits in int32 without overflow.
- **W2.14 byteCount validation** — strict `<` check passes a perfectly-sized PDU; trailing-byte case correct.
- **W2.10 resolved-TTL re-check** — `BcdTagMapBuilder.Build` is called exactly once per PLC at validation; no duplicate work.
- **W2.18 ConnectionOptions validation** — both `MbproxyOptionsValidator` and `ReloadValidator` reject `<= 0`; no bypass path.
- **W3 `HasBadNibble` dedupe** — clean; the codec's internal helper is the single source of truth.
- **W2.15 TCS signalled in every exit path** of `RunSupervisorAsync` — no hang on `WaitForInitialBindAttemptAsync` for the first run. (W4 / NM4 added the re-arm for subsequent Starts.)
- **W2.17 `TransitionTo` lock contract** — both writers use it; `Snapshot` reads under the same lock; no torn triples.
- **`TxIdAllocator.Release` double-call is benign** (`TxIdAllocator.cs:121-129` checks `if (_inUse[id])`); the W1.4 channel drain releasing a TxId already released by the correlation drain is safe.
- **W1.1 in-PDU snapshot consistency** — `OnUpstreamFrameAsync` reads `_ctx.Cache` and `_ctx.TagMap.ResolveCacheTtlMs` non-atomically; the only mid-PDU swap visible would change cache eligibility, not produce corrupted output. Downstream `WithCurrentRequest` snapshots TagMap+Cache for the rewriter, so the rewrite itself is consistent.
- **W2.7 cache-FC byte sourced from post-rewriter buffer** — correct; the rewriter never touches the FC byte but the source must remain `frame[…]` to capture the exception bit.

## Closed

The 2026-05-14 review series — original review → 4 remediation waves → first re-review → wave 4 + followup → second re-review → wave 5 — is now closed. Tests: 387 pass / 0 fail. Three back-to-back race-test runs in isolation all green. Every actionable finding resolved or explicitly accepted with rationale.