dohertj2/wwtools
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

mbproxy: strip historical phase/wave/plan references from source comments

Browse Source 
Comments described the *history* of how the code arrived (phase numbers,
wave IDs, review IDs, dated TODOs) instead of what it does today. That
scaffolding rotted as the codebase evolved. Cleaned 60 source files +
.gitignore; behaviour unchanged (387/387 tests still pass).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-14 13:04:30 -04:00
parent b3b8313e9c
commit 1a2856526a
60 changed files with 750 additions and 811 deletions
Download Patch File Download Diff File Expand all files Collapse all files
mbproxy/src/Mbproxy/Proxy/ProxyWorker.cs
+52 -59
View File
@@ -24,8 +24,8 @@ namespace Mbproxy.Proxy;
/// log <c>mbproxy.startup.ready</c> with bound/configured counts.</item>
/// </list>
///
/// Phase 06: passes the supervisor dictionary to <see cref="ConfigReconciler.Attach"/>
/// after initial startup so hot-reload changes are applied by the reconciler.
/// Passes the supervisor dictionary to <see cref="ConfigReconciler.Attach"/> after
/// initial startup so hot-reload changes are applied by the reconciler.
///
/// Stop: cancels all supervisors in parallel with a 5-second hard deadline.
/// </summary>
@@ -36,30 +36,30 @@ internal sealed partial class ProxyWorker : BackgroundService
private readonly ILogger<ProxyWorker> _logger;
private readonly ILoggerFactory _loggerFactory;
private readonly ConfigReconciler _reconciler;
// Phase 12 (W1.5) — admin endpoint is no longer IHostedService; ProxyWorker drives its
// Admin endpoint is not registered as IHostedService; ProxyWorker drives its
// lifecycle directly so the design's "drain THEN stop admin" ordering is honoured.
//
// Resolved LAZILY (in ExecuteAsync) rather than in the constructor because the DI graph
// is circular: AdminEndpointHost → StatusSnapshotBuilder → ProxyWorker. A constructor
// GetService<AdminEndpointHost>() during ProxyWorker's own construction returns null
// silently. Lazy resolution sidesteps the cycle — by the time ExecuteAsync runs the DI
// container is fully built.
// Resolved LAZILY (in ExecuteAsync) rather than in the constructor because the DI
// graph is circular: AdminEndpointHost → StatusSnapshotBuilder → ProxyWorker. A
// constructor GetService<AdminEndpointHost>() during ProxyWorker's own construction
// returns null silently. Lazy resolution sidesteps the cycle — by the time
// ExecuteAsync runs the DI container is fully built.
private readonly IServiceProvider _services;
private AdminEndpointHost? _admin;
// Phase 06: supervisors are now managed jointly by ProxyWorker (initial bootstrap)
// and ConfigReconciler (subsequent hot-reload changes). The dictionary is shared
// via ConfigReconciler.Attach() after initial startup.
// Supervisors are managed jointly by ProxyWorker (initial bootstrap) and
// ConfigReconciler (subsequent hot-reload changes). The dictionary is shared via
// ConfigReconciler.Attach() after initial startup.
//
// Phase 12 (W2.3) — ConcurrentDictionary because ConfigReconciler mutates this from
// parallel Task.WhenAll continuations (Add/Remove/Restart paths). The outer Apply is
// serialised by a semaphore but the inner per-PLC tasks run concurrently. Status-page
// reads via IReadOnlyDictionary still work without locking.
// ConcurrentDictionary because ConfigReconciler mutates this from parallel
// Task.WhenAll continuations (Add/Remove/Restart paths). The outer Apply is
// serialised by a semaphore but the inner per-PLC tasks run concurrently.
// Status-page reads via IReadOnlyDictionary still work without locking.
private readonly ConcurrentDictionary<string, PlcListenerSupervisor> _supervisors =
new(StringComparer.Ordinal);
/// <summary>
/// Read-only view of the live supervisor dictionary. Consumed by Phase 07's
/// Read-only view of the live supervisor dictionary. Consumed by
/// <see cref="Admin.StatusSnapshotBuilder"/> to enumerate per-PLC state.
/// The caller should read this on the status-page path only (not the hot path).
/// </summary>
@@ -79,7 +79,7 @@ internal sealed partial class ProxyWorker : BackgroundService
_loggerFactory = loggerFactory;
_reconciler = reconciler;
_services = services;
// Phase 12 (W1.5) — admin endpoint resolved lazily in ExecuteAsync (see field comment).
// Admin endpoint resolved lazily in ExecuteAsync (see field comment).
}
protected override async Task ExecuteAsync(CancellationToken stoppingToken)
@@ -107,11 +107,11 @@ internal sealed partial class ProxyWorker : BackgroundService
continue;
}
// Phase 11 — construct a per-PLC response cache only when at least one
// resolved tag opts in (CacheTtlMs > 0). Skipping cache construction for a
// PLC with no cacheable tags keeps the no-cache path free of the eviction
// timer and the per-call resolution cost, preserving "default behaviour =
// Phase 10 unchanged" when no operator has opted any tag in.
// Construct a per-PLC response cache only when at least one resolved tag
// opts in (CacheTtlMs > 0). Skipping cache construction for a PLC with no
// cacheable tags keeps the no-cache path free of the eviction timer and the
// per-call resolution cost, preserving the "no caching" default behaviour
// when no operator has opted any tag in.
var cache = HasAnyCacheableTag(result.Map)
? new ResponseCache(opts.Cache.MaxEntriesPerPlc, opts.Cache.EvictionIntervalMs)
: null;
@@ -144,9 +144,9 @@ internal sealed partial class ProxyWorker : BackgroundService
resilienceOpts.ListenerRecovery,
_loggerFactory.CreateLogger($"Mbproxy.Proxy.ListenerRecovery.{plc.Name}"));
// Phase 10 — give the supervisor a live accessor for ReadCoalescingOptions
// so a hot-reload of `Mbproxy.Resilience.ReadCoalescing.Enabled` propagates
// to the multiplexer's per-PDU coalescing decision.
// Give the supervisor a live accessor for ReadCoalescingOptions so a
// hot-reload of `Mbproxy.Resilience.ReadCoalescing.Enabled` propagates to
// the multiplexer's per-PDU coalescing decision.
Func<ReadCoalescingOptions> coalescingAccessor =
() => _options.CurrentValue.Resilience.ReadCoalescing;
@@ -166,13 +166,13 @@ internal sealed partial class ProxyWorker : BackgroundService
_supervisors[plc.Name] = supervisor;
}
// ── Phase 06: wire reconciler BEFORE starting supervisors ─────────────────
// ── Wire reconciler BEFORE starting supervisors ──────────────────────────
// Attach hands the reconciler the authoritative supervisor dictionary and the
// initial options snapshot. The reconciler won't process OnChange events until
// after this call — the brief window between Attach and first supervisor start
// is safe because the channel signal only enqueues; apply runs asynchronously.
// Phase 12 (W2.1) — also pass the live coalescing accessor so reconciler-built
// supervisors (add/restart paths) honour hot-reloaded ReadCoalescing values.
// Pass the live coalescing accessor so reconciler-built supervisors
// (add/restart paths) honour hot-reloaded ReadCoalescing values.
Func<ReadCoalescingOptions> reconcilerCoalescingAccessor =
() => _options.CurrentValue.Resilience.ReadCoalescing;
_reconciler.Attach(_supervisors, opts, reconcilerCoalescingAccessor);
@@ -213,10 +213,10 @@ internal sealed partial class ProxyWorker : BackgroundService
int boundCount = _supervisors.Values.Count(s => s.Snapshot().State == SupervisorState.Bound);
LogStartupReady(_logger, boundCount, plcsConfigured);
// Phase 12 (W1.5) — start the admin endpoint AFTER listeners are bound so the
// status page can never observe the service in a "no PLCs configured yet" state.
// The admin endpoint is no longer registered as IHostedService (the host's reverse
// stop order would tear it down BEFORE drain). ProxyWorker drives both ends.
// Start the admin endpoint AFTER listeners are bound so the status page can
// never observe the service in a "no PLCs configured yet" state. The admin
// endpoint is not registered as IHostedService (the host's reverse stop order
// would tear it down BEFORE drain) — ProxyWorker drives both ends.
//
// Resolution happens here, not in the constructor — the DI graph is circular
// (admin → StatusSnapshotBuilder → ProxyWorker) and a constructor-time lookup
@@ -235,10 +235,9 @@ internal sealed partial class ProxyWorker : BackgroundService
}
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.
// Surface the absence. The lazy lookup returns null silently if
// AddMbproxyAdmin() is missing from Program.cs; 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.");
@@ -250,8 +249,7 @@ internal sealed partial class ProxyWorker : BackgroundService
}
/// <summary>
/// Phase 12 (W1.5) — graceful shutdown sequence (replaces the deleted
/// <c>ShutdownCoordinator</c>):
/// Graceful shutdown sequence:
/// <list type="number">
/// <item>Cancel <see cref="ExecuteAsync"/> via <c>base.StopAsync</c>.</item>
/// <item><b>Snapshot</b> per-PLC in-flight counts BEFORE stopping supervisors —
@@ -263,10 +261,7 @@ internal sealed partial class ProxyWorker : BackgroundService
/// stop is the actual drain — it cancels the listener, which exits its
/// accept loop, which disposes the multiplexer, which cascades all attached
/// pipes. There is no separate "drain in-flight" phase because there is
/// nothing to drain that wouldn't be killed by the supervisor stop itself
/// (the original Phase-08 ShutdownCoordinator's drain loop had this same
/// shape and was structurally always-zero — call out from
/// codereviews/2026-05-14/ReReviewAfterRemediation.md NC1).</item>
/// nothing to drain that wouldn't be killed by the supervisor stop itself.</item>
/// <item>Stop the admin endpoint LAST so the status page survives the supervisor
/// stop phase and operators can observe the live state right up to shutdown.</item>
/// <item>Dispose every supervisor to release sockets, channels, and watchdog timers.</item>
@@ -277,16 +272,15 @@ internal sealed partial class ProxyWorker : BackgroundService
/// </summary>
public override async Task StopAsync(CancellationToken cancellationToken)
{
// Phase 12 (W5 / m2) — snapshot in-flight BEFORE base.StopAsync so the field
// matches its name: "the count at the moment the host signalled stop", not "the
// count at the moment we got around to computing it." `base.StopAsync` cancels the
// ExecuteAsync stoppingToken; in the milliseconds before it returns, in-flight
// requests whose responses arrive will be removed from _correlation and the
// watchdog can clear stale entries — the count would otherwise drift downward.
// Snapshot in-flight BEFORE base.StopAsync so the field matches its name: "the
// count at the moment the host signalled stop", not "the count at the moment we
// got around to computing it." `base.StopAsync` cancels the ExecuteAsync
// stoppingToken; in the milliseconds before it returns, in-flight requests
// whose responses arrive will be removed from _correlation and the watchdog can
// clear stale entries — the count would otherwise drift downward.
//
// Phase 12 (W4 / NC1) — must run BEFORE supervisor stop too: after
// supervisor.StopAsync, multiplexers are disposed and CountInFlight returns 0
// unconditionally (the original ShutdownCoordinator had the same defect).
// Must run BEFORE supervisor stop too: after supervisor.StopAsync, multiplexers
// are disposed and CountInFlight returns 0 unconditionally.
int inFlightAtCancel = CountInFlight();
// Cancel ExecuteAsync first.
@@ -294,9 +288,9 @@ internal sealed partial class ProxyWorker : BackgroundService
var sw = Stopwatch.StartNew();
// Phase 12 (W2.20) — supervisor stop deadline read from the live config so a
// hot-reloaded GracefulShutdownTimeoutMs is honoured. Supervisor stop is the
// drain: cancelling the supervisor cancels the listener, which exits accept, which
// Supervisor stop deadline read from the live config so a hot-reloaded
// GracefulShutdownTimeoutMs is honoured. Supervisor stop is the drain:
// cancelling the supervisor cancels the listener, which exits accept, which
// disposes the multiplexer, which cascades all attached pipes.
int gracefulMs = _options.CurrentValue.Connection.GracefulShutdownTimeoutMs;
@@ -352,11 +346,11 @@ internal sealed partial class ProxyWorker : BackgroundService
// ── Logging ───────────────────────────────────────────────────────────────────────────
/// <summary>
/// Phase 11 — returns <c>true</c> when at least one BcdTag in the resolved map has a
/// positive <see cref="BcdTag.CacheTtlMs"/>. A PLC with no cacheable tags skips the
/// Returns <c>true</c> when at least one BcdTag in the resolved map has a positive
/// <see cref="BcdTag.CacheTtlMs"/>. A PLC with no cacheable tags skips the
/// <see cref="Mbproxy.Proxy.Cache.ResponseCache"/> entirely (no eviction timer, no
/// per-call cache resolution cost), so the default-OFF deployment is byte-identical
/// to a Phase-10 deployment.
/// per-call cache resolution cost), so the default-OFF deployment runs the
/// no-cache code path.
/// </summary>
private static bool HasAnyCacheableTag(BcdTagMap map)
{
@@ -375,7 +369,6 @@ internal sealed partial class ProxyWorker : BackgroundService
Message = "Failed to bind listener: Plc={Plc} Port={Port} Reason={Reason}")]
private static partial void LogBindFailed(ILogger logger, string plc, int port, string reason);
// Phase 12 (W1.5) — moved here from the deleted ShutdownCoordinator.
[LoggerMessage(EventId = 80, EventName = "mbproxy.shutdown.complete",
Level = LogLevel.Information,
Message = "Graceful shutdown complete: InFlightAtCancel={InFlightAtCancel} ElapsedMs={ElapsedMs}")]