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

Resolve all 46 low-severity architecture review findings

Browse Source
This commit is contained in:
Joseph Doherty
2026-09-09 22:56:36 -04:00
parent fef3e7675f
commit 9ee402bbcd
139 changed files with 2621 additions and 1082 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/ZB.MOM.WW.ScadaBridge.Communication/Grpc/CentralControlGrpcService.cs
+6 -12
View File
@@ -39,17 +39,13 @@ namespace ZB.MOM.WW.ScadaBridge.Communication.Grpc;
/// <see cref="SiteStreamGrpcServer.AuditIngestAskTimeout"/>, so the inner Ask expired at the same
/// instant as the outer one and could only add latency plus a
/// <see cref="Akka.Actor.Status.Failure"/> repackaging. The proxy now arrives through
/// <see cref="SetAuditIngestActor"/>, exactly as <see cref="SiteStreamGrpcServer"/> has always
/// taken it, and the relay handlers were deleted.
/// <see cref="SetAuditIngestActor"/>; the redundant actor relay handlers were deleted.
/// </para>
/// <para>
/// <b>Fault semantics deliberately differ from <see cref="SiteStreamGrpcServer"/>'s ingest
/// RPCs.</b> That server answers a failed audit ingest with an EMPTY <c>IngestAck</c>; this one
/// fails the call with a non-OK status. Both leave the site's rows <c>Pending</c> for the next
/// drain, so the outcome is the same — but this service replaced the ClusterClient path, whose
/// documented behaviour was to propagate the fault, and preserving that keeps a lost batch
/// visible as a failure rather than as a successful call that acked nothing. A proxy that has
/// not been handed over yet is NOT a fault and still answers with an empty ack.
/// <b>Fault semantics.</b> Failed central ingest returns a non-OK status, leaving the site's
/// rows Pending and making a lost batch visible to the next drain. A proxy not handed over
/// yet returns an empty ack. The legacy SiteStreamService ingest methods are retired and
/// always return Unimplemented; they are not an alternate ingestion path.
/// </para>
/// <para>
/// <b>Status mapping, and why it is not uniform.</b> A site transport may safely re-send a call
@@ -126,9 +122,7 @@ public sealed class CentralControlGrpcService : CentralControlService.CentralCon
/// <summary>
/// Hands the central <c>AuditLogIngestActor</c> singleton proxy to the service so the two
/// ingest RPCs can Ask it DIRECTLY. Mirrors
/// <see cref="SiteStreamGrpcServer.SetAuditIngestActor"/> — the site-stream server has always
/// dispatched straight to the singleton, and this service now matches it.
/// ingest RPCs can Ask it directly. SiteStreamService retains only Unimplemented wire stubs.
/// </summary>
/// <remarks>
/// Removing the <c>CentralCommunicationActor</c> relay removes an actor hop, a second Ask,
src/ZB.MOM.WW.ScadaBridge.Communication/Grpc/SitePairChannelProvider.cs
+99 -95
View File
@@ -52,9 +52,7 @@ public sealed class SitePairChannelProvider : IDisposable
/// <summary>
/// How long a replaced/removed channel is PARKED before it is disposed (arch-review
/// 2026-08-19 P3-2). Sized to exceed the longest command deadline
/// (<c>CommunicationOptions.DeploymentTimeout</c> and friends, minutes at most), so no call
/// that was already in flight when the channel was replaced can still be using it.
/// 2026-08-19 P3-2). Active calls retain the channel beyond this minimum until completion.
/// </summary>
internal static readonly TimeSpan ChannelDisposalGrace = TimeSpan.FromMinutes(5);
@@ -68,15 +66,16 @@ public sealed class SitePairChannelProvider : IDisposable
/// <summary>
/// Channels replaced by <see cref="UpdateSite"/> or dropped by <see cref="RemoveSite"/>,
/// each with the instant it becomes safe to dispose. FIFO order is disposal order because
/// the grace window is constant.
/// each with the earliest idle disposal instant. Active-call retention can extend that instant.
/// </summary>
private readonly ConcurrentQueue<(GrpcChannel Channel, DateTime DisposeAfterUtc)> _graveyard = new();
private readonly ConcurrentDictionary<GrpcChannel, int> _activeCalls = new();
private readonly CancellationTokenSource _shutdown = new();
private Task? _failbackLoop;
private readonly object _loopGate = new();
private bool _disposed;
private volatile bool _disposed;
private readonly object _lifecycleGate = new();
/// <summary>Creates the provider.</summary>
/// <param name="pskProvider">Resolves each site's preshared key for channel credentials.</param>
@@ -120,38 +119,43 @@ public sealed class SitePairChannelProvider : IDisposable
/// <param name="nodeBEndpoint">NodeB gRPC base address, or null when unconfigured.</param>
public void UpdateSite(string siteId, string? nodeAEndpoint, string? nodeBEndpoint)
{
ArgumentException.ThrowIfNullOrWhiteSpace(siteId);
SweepGraveyard();
var pair = _sites.GetOrAdd(siteId, id => new SitePair(id));
lock (pair.Gate)
lock (_lifecycleGate)
{
var changedA = !string.Equals(pair.NodeAEndpoint, nodeAEndpoint, StringComparison.Ordinal);
var changedB = !string.Equals(pair.NodeBEndpoint, nodeBEndpoint, StringComparison.Ordinal);
if (!changedA && !changedB)
ObjectDisposedException.ThrowIf(_disposed, this);
ArgumentException.ThrowIfNullOrWhiteSpace(siteId);
SweepGraveyard();
var pair = _sites.GetOrAdd(siteId, id => new SitePair(id));
lock (pair.Gate)
{
return;
var changedA = !string.Equals(pair.NodeAEndpoint, nodeAEndpoint, StringComparison.Ordinal);
var changedB = !string.Equals(pair.NodeBEndpoint, nodeBEndpoint, StringComparison.Ordinal);
if (!changedA && !changedB)
{
return;
}
if (changedA)
{
Park(pair.ChannelA);
pair.ChannelA = string.IsNullOrWhiteSpace(nodeAEndpoint) ? null : BuildChannel(nodeAEndpoint, siteId);
pair.NodeAEndpoint = nodeAEndpoint;
}
if (changedB)
{
Park(pair.ChannelB);
pair.ChannelB = string.IsNullOrWhiteSpace(nodeBEndpoint) ? null : BuildChannel(nodeBEndpoint, siteId);
pair.NodeBEndpoint = nodeBEndpoint;
}
// Addresses changed — return to the preferred node and reset failback backoff.
pair.CurrentIsA = pair.ChannelA is not null;
pair.ResetFailback();
_logger.LogInformation(
"gRPC channel pair for site {SiteId} refreshed (nodeA={HasA}, nodeB={HasB})",
siteId, pair.ChannelA is not null, pair.ChannelB is not null);
}
if (changedA)
{
Park(pair.ChannelA);
pair.ChannelA = string.IsNullOrWhiteSpace(nodeAEndpoint) ? null : BuildChannel(nodeAEndpoint, siteId);
pair.NodeAEndpoint = nodeAEndpoint;
}
if (changedB)
{
Park(pair.ChannelB);
pair.ChannelB = string.IsNullOrWhiteSpace(nodeBEndpoint) ? null : BuildChannel(nodeBEndpoint, siteId);
pair.NodeBEndpoint = nodeBEndpoint;
}
// Addresses changed — return to the preferred node and reset failback backoff.
pair.CurrentIsA = pair.ChannelA is not null;
pair.ResetFailback();
_logger.LogInformation(
"gRPC channel pair for site {SiteId} refreshed (nodeA={HasA}, nodeB={HasB})",
siteId, pair.ChannelA is not null, pair.ChannelB is not null);
}
}
@@ -162,24 +166,29 @@ public sealed class SitePairChannelProvider : IDisposable
/// <param name="siteId">Site identifier to remove.</param>
public void RemoveSite(string siteId)
{
if (string.IsNullOrWhiteSpace(siteId))
lock (_lifecycleGate)
{
return;
}
SweepGraveyard();
if (_sites.TryRemove(siteId, out var pair))
{
lock (pair.Gate)
ObjectDisposedException.ThrowIf(_disposed, this);
if (string.IsNullOrWhiteSpace(siteId))
{
Park(pair.ChannelA);
Park(pair.ChannelB);
pair.ChannelA = null;
pair.ChannelB = null;
return;
}
_pskProvider.Invalidate(siteId);
_logger.LogInformation("gRPC channel pair for site {SiteId} removed", siteId);
SweepGraveyard();
if (_sites.TryRemove(siteId, out var pair))
{
lock (pair.Gate)
{
Park(pair.ChannelA);
Park(pair.ChannelB);
pair.ChannelA = null;
pair.ChannelB = null;
}
_pskProvider.Invalidate(siteId);
_logger.LogInformation("gRPC channel pair for site {SiteId} removed", siteId);
}
}
}
@@ -219,6 +228,8 @@ public sealed class SitePairChannelProvider : IDisposable
// Recompute which node `primary` actually is (the current side may be null).
primaryIsA = ReferenceEquals(primary, pair.ChannelA);
secondary = primaryIsA ? pair.ChannelB : pair.ChannelA;
_activeCalls.AddOrUpdate(primary, 1, (_, count) => count + 1);
if (secondary is not null) _activeCalls.AddOrUpdate(secondary, 1, (_, count) => count + 1);
}
try
@@ -242,6 +253,12 @@ public sealed class SitePairChannelProvider : IDisposable
throw GrpcRetrySafety.OutcomeUnknown(retryError);
}
}
finally
{
_activeCalls.AddOrUpdate(primary, 0, (_, count) => count - 1);
if (secondary is not null) _activeCalls.AddOrUpdate(secondary, 0, (_, count) => count - 1);
if (_disposed) SweepGraveyard(force: true);
}
}
/// <summary>
@@ -360,13 +377,8 @@ public sealed class SitePairChannelProvider : IDisposable
/// and the command simply failed.
/// </para>
/// <para>
/// <b>Why a timed park rather than reference counting.</b> A refcount would have to be taken
/// and released around every <c>ExecuteAsync</c> call — bookkeeping on the hot path, plus a
/// quiescence handshake — to buy nothing a fixed window does not already give: the window
/// only has to outlast the longest command deadline. It also matches the precedent set by
/// <see cref="SiteStreamGrpcClientFactory"/>, which likewise keeps a superseded endpoint's
/// channel alive rather than disposing it under a concurrent session. Cost is a handful of
/// idle HTTP/2 channels for at most <see cref="ChannelDisposalGrace"/>.
/// Active calls retain both selected channels until completion, including the retry channel.
/// The grace is only an idle-channel minimum; dynamic WaitForAttribute deadlines can exceed it.
/// </para>
/// </summary>
/// <param name="channel">The channel to park; null is ignored.</param>
@@ -383,25 +395,27 @@ public sealed class SitePairChannelProvider : IDisposable
/// path (<see cref="UpdateSite"/>/<see cref="RemoveSite"/>) and from the failback loop, so a
/// park always drains even on a provider that never runs a call again.
/// </summary>
/// <param name="force">True to dispose every parked channel regardless of its window (shutdown, and tests).</param>
/// <param name="force">True to ignore idle grace (shutdown and tests); active calls always retain their channels.</param>
internal void SweepGraveyard(bool force = false)
{
var now = DateTime.UtcNow;
while (_graveyard.TryPeek(out var parked) && (force || now >= parked.DisposeAfterUtc))
lock (_lifecycleGate)
{
if (!_graveyard.TryDequeue(out parked))
var now = DateTime.UtcNow;
var count = _graveyard.Count;
// Visit each parked channel once: an occupied channel must not starve later entries.
for (var i = 0; i < count && _graveyard.TryDequeue(out var parked); i++)
{
break;
if ((!force && now < parked.DisposeAfterUtc)
|| _activeCalls.GetValueOrDefault(parked.Channel) > 0)
{
_graveyard.Enqueue(parked);
continue;
}
_activeCalls.TryRemove(parked.Channel, out _);
try { parked.Channel.Dispose(); }
catch (Exception ex) { _logger.LogDebug(ex, "Disposing a parked gRPC channel faulted"); }
}
try
{
parked.Channel.Dispose();
}
catch (Exception ex)
{
_logger.LogDebug(ex, "Disposing a parked gRPC channel faulted");
}
}
}
@@ -461,39 +475,29 @@ public sealed class SitePairChannelProvider : IDisposable
/// <inheritdoc />
public void Dispose()
{
if (_disposed)
lock (_lifecycleGate)
{
return;
}
_disposed = true;
_shutdown.Cancel();
try
{
_failbackLoop?.Wait(TimeSpan.FromSeconds(2));
}
catch
{
// Best-effort loop drain on shutdown.
}
_shutdown.Dispose();
foreach (var pair in _sites.Values)
{
lock (pair.Gate)
if (_disposed) return;
_disposed = true;
foreach (var pair in _sites.Values)
{
pair.ChannelA?.Dispose();
pair.ChannelB?.Dispose();
lock (pair.Gate)
{
Park(pair.ChannelA);
Park(pair.ChannelB);
pair.ChannelA = null;
pair.ChannelB = null;
}
}
_sites.Clear();
}
_sites.Clear();
// Process shutdown — nothing can still be in flight, so drain the graveyard regardless
// of the remaining grace on each entry.
_shutdown.Cancel();
try { _failbackLoop?.Wait(TimeSpan.FromSeconds(2)); }
catch { /* shutdown is best effort; active calls retain their channels */ }
_shutdown.Dispose();
SweepGraveyard(force: true);
}
/// <summary>Per-site mutable channel state, guarded by <see cref="Gate"/>.</summary>
private sealed class SitePair(string siteId)
{
public string SiteId { get; } = siteId;
src/ZB.MOM.WW.ScadaBridge.Communication/Grpc/SiteStreamGrpcClient.cs
+9 -3
View File
@@ -82,6 +82,7 @@ public class SiteStreamGrpcClient : IAsyncDisposable, IDisposable
ISitePskProvider? pskProvider,
string? siteIdentifier)
{
ConnectedHeaderTimeout = options.ConnectedHeaderTimeout;
Endpoint = endpoint;
KeepAlivePingDelay = options.GrpcKeepAlivePingDelay;
KeepAlivePingTimeout = options.GrpcKeepAlivePingTimeout;
@@ -122,7 +123,12 @@ public class SiteStreamGrpcClient : IAsyncDisposable, IDisposable
/// Unsubscribe and Dispose behavior without needing a real endpoint.
/// </summary>
/// <returns>A <see cref="SiteStreamGrpcClient"/> with no channel or client, for testing only.</returns>
internal static SiteStreamGrpcClient CreateForTesting() => new();
internal static SiteStreamGrpcClient CreateForTesting(CommunicationOptions? options = null) => new(options ?? new CommunicationOptions());
private SiteStreamGrpcClient(CommunicationOptions options)
{
ConnectedHeaderTimeout = options.ConnectedHeaderTimeout;
}
/// <summary>
/// Registers a CancellationTokenSource for a correlation ID. Test-only.
@@ -457,7 +463,7 @@ public class SiteStreamGrpcClient : IAsyncDisposable, IDisposable
/// with its first message would otherwise hold the connected signal — and with it the
/// aggregator's re-seed — for as long as the site happens to be quiet.
/// </summary>
internal static TimeSpan ConnectedHeaderTimeout { get; set; } = TimeSpan.FromSeconds(10);
internal TimeSpan ConnectedHeaderTimeout { get; } = TimeSpan.FromSeconds(10);
/// <summary>
/// Awaits the call's response headers, bounded by <see cref="ConnectedHeaderTimeout"/>.
@@ -475,7 +481,7 @@ public class SiteStreamGrpcClient : IAsyncDisposable, IDisposable
/// timeout was added for is covered by the reconcile backstop.
/// </para>
/// </summary>
private static async Task<bool> AwaitHeadersAsync(
private async Task<bool> AwaitHeadersAsync(
AsyncServerStreamingCall<SiteStreamEvent> call, CancellationToken ct)
{
var headers = call.ResponseHeadersAsync;
src/ZB.MOM.WW.ScadaBridge.Communication/Grpc/SiteStreamGrpcServer.cs
+17 -128
View File
@@ -38,11 +38,6 @@ public class SiteStreamGrpcServer : SiteStreamService.SiteStreamServiceBase
// true, never reset (the server is single-lifetime per host).
private volatile bool _shuttingDown;
private long _actorCounter;
// Central-side ingest actor proxy. Set by the host
// after the cluster singleton starts. When null the
// IngestAuditEvents RPC replies with an empty IngestAck so sites can
// safely retry — wiring-incomplete is treated as transient, never fatal.
private IActorRef? _auditIngestActor;
// Ask timeout is 30 s. The ingest actor's repo
// calls are sub-100 ms in steady state; a generous timeout absorbs a slow
// MSSQL connection without surfacing as a gRPC failure on a healthy site.
@@ -153,26 +148,12 @@ public class SiteStreamGrpcServer : SiteStreamService.SiteStreamServiceBase
_ready = true;
}
/// <summary>
/// Hands the central-side <c>AuditLogIngestActor</c> proxy to the gRPC
/// server so the <see cref="IngestAuditEvents"/> RPC can route incoming
/// site batches. Mirrors the way
/// <c>CommunicationService.SetNotificationOutbox</c> takes the Notification
/// Outbox singleton proxy. The actor is supplied after the cluster
/// singleton starts.
/// </summary>
/// <param name="proxy">The audit log ingest actor proxy.</param>
public void SetAuditIngestActor(IActorRef proxy)
{
_auditIngestActor = proxy;
}
/// <summary>
/// Hands the site-local <see cref="ISiteAuditQueue"/> (the same
/// <c>SqliteAuditWriter</c> singleton that backs <see cref="IAuditWriter"/>
/// on the script thread) to the gRPC server so the
/// <see cref="PullAuditEvents"/> RPC can serve central's reconciliation
/// pulls. Mirrors <see cref="SetAuditIngestActor"/>: wired post-construction
/// pulls. Wired post-construction
/// because the queue and the gRPC server are both DI singletons brought up
/// in independent orders on site startup.
/// </summary>
@@ -535,111 +516,18 @@ public class SiteStreamGrpcServer : SiteStreamService.SiteStreamServiceBase
}
}
/// <inheritdoc />
public override async Task<IngestAck> IngestAuditEvents(
AuditEventBatch request,
ServerCallContext context)
{
// Empty batch is a no-op; reply immediately so the client moves on.
if (request.Events.Count == 0)
{
return new IngestAck();
}
/// <summary>Retained wire method; ingest is served only by CentralControlService.</summary>
public override Task<IngestAck> IngestAuditEvents(AuditEventBatch request, ServerCallContext context)
=> Task.FromException<IngestAck>(new RpcException(new GrpcStatus(
StatusCode.Unimplemented, "Use CentralControlService.IngestAuditEvents on central.")));
var actor = _auditIngestActor;
if (actor is null)
{
// Wiring incomplete (host startup race). Sites treat an empty
// ack as "nothing was acked, leave rows Pending, retry next
// drain" — exactly the right behaviour during host bring-up.
_logger.LogWarning(
"IngestAuditEvents received {Count} events before SetAuditIngestActor was called; returning empty ack.",
request.Events.Count);
return new IngestAck();
}
/// <summary>Retained wire method; combined ingest is served only by CentralControlService.</summary>
public override Task<IngestAck> IngestCachedTelemetry(CachedTelemetryBatch request, ServerCallContext context)
=> Task.FromException<IngestAck>(new RpcException(new GrpcStatus(
StatusCode.Unimplemented, "Use CentralControlService.IngestCachedTelemetry on central.")));
var entities = new List<AuditEvent>(request.Events.Count);
foreach (var dto in request.Events)
{
entities.Add(AuditEventDtoMapper.FromDto(dto));
}
var cmd = new IngestAuditEventsCommand(entities);
IngestAuditEventsReply reply;
try
{
reply = await actor.Ask<IngestAuditEventsReply>(
cmd, AuditIngestAskTimeout, context.CancellationToken);
}
catch (Exception ex)
{
// Audit ingest is best-effort; failing this RPC at the gRPC layer
// would surface as a transport error and force the site to retry
// (which it would do anyway). Logging + an empty ack keeps the
// semantics consistent with the "wiring incomplete" path above.
_logger.LogError(ex,
"AuditLogIngestActor Ask failed for batch of {Count} events; returning empty ack.",
request.Events.Count);
return new IngestAck();
}
var ack = new IngestAck();
foreach (var id in reply.AcceptedEventIds)
{
ack.AcceptedEventIds.Add(id.ToString());
}
return ack;
}
/// <inheritdoc />
public override async Task<IngestAck> IngestCachedTelemetry(
CachedTelemetryBatch request,
ServerCallContext context)
{
if (request.Packets.Count == 0)
{
return new IngestAck();
}
var actor = _auditIngestActor;
if (actor is null)
{
_logger.LogWarning(
"IngestCachedTelemetry received {Count} packets before SetAuditIngestActor was called; returning empty ack.",
request.Packets.Count);
return new IngestAck();
}
var entries = new List<CachedTelemetryEntry>(request.Packets.Count);
foreach (var packet in request.Packets)
{
var auditEvent = AuditEventDtoMapper.FromDto(packet.AuditEvent);
var siteCall = SiteCallDtoMapper.FromDto(packet.Operational);
entries.Add(new CachedTelemetryEntry(auditEvent, siteCall));
}
var cmd = new IngestCachedTelemetryCommand(entries);
IngestCachedTelemetryReply reply;
try
{
reply = await actor.Ask<IngestCachedTelemetryReply>(
cmd, AuditIngestAskTimeout, context.CancellationToken);
}
catch (Exception ex)
{
_logger.LogError(ex,
"AuditLogIngestActor Ask failed for combined telemetry batch of {Count} packets; returning empty ack.",
request.Packets.Count);
return new IngestAck();
}
var ack = new IngestAck();
foreach (var id in reply.AcceptedEventIds)
{
ack.AcceptedEventIds.Add(id.ToString());
}
return ack;
}
/// <summary>Maximum rows requested by one reconciliation pull, before the wire-byte budget.</summary>
public const int MaxReconciliationRowsPerPull = 2000;
/// <summary>Flow Debugger PullTraceEvents: server-side row cap (design 2026-09-05 §5.4).</summary>
public const int MaxTraceEventsPerPull = 2000;
@@ -694,7 +582,8 @@ public class SiteStreamGrpcServer : SiteStreamService.SiteStreamServiceBase
IReadOnlyList<AuditEvent> events;
try
{
events = await queue.ReadByExecutionIdsAsync(ids, since, request.MaxEvents, context.CancellationToken);
events = await queue.ReadByExecutionIdsAsync(ids, since, request.MaxEvents, context.CancellationToken,
string.IsNullOrEmpty(request.AfterId) ? null : request.AfterId);
}
catch (Exception ex)
{
@@ -725,13 +614,13 @@ public class SiteStreamGrpcServer : SiteStreamService.SiteStreamServiceBase
return new PullAuditEventsResponse();
}
if (request.BatchSize <= 0)
if (request.BatchSize <= 0 || request.BatchSize > MaxReconciliationRowsPerPull)
{
// Mirrors the SubscribeInstance guard: reject malformed requests
// cleanly with InvalidArgument so the caller doesn't see a generic
// RpcException from the underlying SQLite parameter validation.
throw new RpcException(new GrpcStatus(
StatusCode.InvalidArgument, "batch_size must be > 0"));
StatusCode.InvalidArgument, $"batch_size must be 1..{MaxReconciliationRowsPerPull}"));
}
// sinceUtc defaults to DateTime.MinValue when the wrapper is absent —
@@ -818,13 +707,13 @@ public class SiteStreamGrpcServer : SiteStreamService.SiteStreamServiceBase
return new PullSiteCallsResponse();
}
if (request.BatchSize <= 0)
if (request.BatchSize <= 0 || request.BatchSize > MaxReconciliationRowsPerPull)
{
// Mirrors PullAuditEvents: reject malformed requests cleanly with
// InvalidArgument so the caller doesn't see a generic RpcException
// from the underlying SQLite parameter validation.
throw new RpcException(new GrpcStatus(
StatusCode.InvalidArgument, "batch_size must be > 0"));
StatusCode.InvalidArgument, $"batch_size must be 1..{MaxReconciliationRowsPerPull}"));
}
// since_utc defaults to DateTime.MinValue when the wrapper is absent —