mbproxy: resolve remaining items from ReReviewAfterRemediation.md

Closes the latent + minor + test-discipline items left after Wave 4. Updates
the re-review doc with a final resolution table — every actionable finding
now marked Resolved or Accepted with rationale.

NM3 — _supervisorCts leaks on re-Start
  StartAsync now disposes the previous CTS before reassigning. Idempotent:
  a try/catch (ObjectDisposedException) covers the very-first-Start case
  where the field-init CTS is still fresh.

NM4 — W2.15 TCS is single-shot
  _firstAttemptCompleted is no longer readonly; StartAsync re-creates it
  after the W2.16 guard so a re-Started supervisor's
  WaitForInitialBindAttemptAsync doesn't observe the previous run's signal.

Nm6 — _admin GetService<> returns null silently
  ProxyWorker.ExecuteAsync now logs a Warning when admin isn't registered.
  Preserves the loud-failure intent from the original IHostedService
  registration without forcing test hosts to wire admin.

Nm7 — AdminEndpointHost.DisposeAsync no double-dispose guard
  Added a volatile bool _disposed flag with an early-return at the top of
  DisposeAsync. Symmetry with PlcMultiplexer; protects against
  ProxyWorker.StopAsync explicitly disposing then DI disposing the singleton
  again on host shutdown.

T3 — RemoveInheritedAppsettings only fires on Build
  AfterTargets="Build;Publish" + a second Delete against $(PublishDir)
  so a `dotnet publish` against the test csproj doesn't ship the example
  PLCs from the linked install template.

T4 — Stale TryAttachOrCreate_*_ReturnsTrue_* test method names
  Renamed to AttachOrCreate_*_WasNew{True,False} after W3 dropped the bool
  return.

Accepted (with rationale documented in ReReviewAfterRemediation.md):
  Nm2 — CoalescedHit semantic is per-design
  Nm4 — _lastBindError preservation on clean exit is intentional forensics
  Nm5 — EventLogBridge has no injectable logger
  Nm8 — Cosmetic log noise
  T1  — Reflection on private fields documented as maintenance trap

Tests: 387 pass / 0 fail.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-14 07:02:21 -04:00
parent 7a435957ee
commit 9251c564c1
6 changed files with 134 additions and 87 deletions
+13 -1
View File
@@ -44,6 +44,13 @@ internal sealed partial class AdminEndpointHost : IAsyncDisposable
// Protects concurrent Start/Stop calls (hot-reload + StopAsync racing).
private readonly SemaphoreSlim _lock = new(1, 1);
// Phase 12 (W4 / Nm7) — idempotency flag for DisposeAsync. ProxyWorker.StopAsync
// calls our StopAsync explicitly; the DI container then disposes the singleton on
// host shutdown. Without this flag the second pass would Dispose `_lock` twice and
// re-dispose the change registration (both currently safe but symmetry with
// PlcMultiplexer prevents future regression).
private volatile bool _disposed;
// Current configured port — used to detect changes on hot-reload.
private int _currentPort;
@@ -199,14 +206,19 @@ internal sealed partial class AdminEndpointHost : IAsyncDisposable
public async ValueTask DisposeAsync()
{
if (_disposed) return;
_disposed = true;
_optionsChangeRegistration?.Dispose();
_lock.Dispose();
_optionsChangeRegistration = null;
if (_app is { } app)
{
_app = null;
await app.DisposeAsync().ConfigureAwait(false);
}
try { _lock.Dispose(); } catch (ObjectDisposedException) { /* race-safe */ }
}
// ── Logging ──────────────────────────────────────────────────────────────
+10
View File
@@ -233,6 +233,16 @@ internal sealed partial class ProxyWorker : BackgroundService
_logger.LogError(ex, "Admin endpoint failed to start: {Message}", ex.Message);
}
}
else
{
// Phase 12 (W4 / Nm6) — surface the absence. The previous IHostedService
// registration would have hard-errored in DI if AddMbproxyAdmin() was missing
// from Program.cs; the W1.5 lazy lookup returns null silently. A single warning
// makes a botched composition observable without blocking startup.
_logger.LogWarning(
"Admin endpoint not registered (AddMbproxyAdmin() missing from composition). " +
"Status page will be unavailable; service continues without it.");
}
// ── 6. Keep the worker alive until the host signals stop ─────────────────────
// Supervisors run their own background loops; ExecuteAsync just waits.
@@ -71,7 +71,13 @@ internal sealed partial class PlcListenerSupervisor : IAsyncDisposable
// 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(
//
// Phase 12 (W4 / NM4) — non-readonly so StartAsync can re-arm it for a re-Started
// supervisor. Without re-arming, a restart-after-stop scenario would have
// WaitForInitialBindAttemptAsync return immediately on the previous run's signal,
// never observing the new run's bind status. No production caller currently re-Starts,
// but the supervisor's state machine should be consistent.
private TaskCompletionSource _firstAttemptCompleted = new(
TaskCreationOptions.RunContinuationsAsynchronously);
// ── Public surface ────────────────────────────────────────────────────────────────────
@@ -132,16 +138,28 @@ 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.
// supervisor. After Stop the state machine returns to Stopped and StartAsync
// can re-arm; W4/NM3+NM4 below ensure the per-Start state (CTS, TCS) is fresh
// each time so no leak or stale signal carries across cycles.
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.");
// Phase 12 (W4 / NM3) — dispose the previous CTS before reassigning. The original
// code overwrote _supervisorCts unconditionally, leaking the prior CTS on every
// re-Start cycle (and any registrations linked to it). Idempotent: ObjectDisposed
// catch covers the very-first-Start case where the field-init CTS is still fresh.
try { _supervisorCts.Dispose(); } catch (ObjectDisposedException) { /* fresh */ }
_supervisorCts = CancellationTokenSource.CreateLinkedTokenSource(ct);
// Phase 12 (W4 / NM4) — re-arm the first-attempt TCS so a re-Started supervisor
// doesn't immediately observe the previous run's signal in
// WaitForInitialBindAttemptAsync.
_firstAttemptCompleted = new TaskCompletionSource(
TaskCreationOptions.RunContinuationsAsynchronously);
_supervisorTask = Task.Run(() => RunSupervisorAsync(_supervisorCts.Token), CancellationToken.None);
return Task.CompletedTask;
}