Resolve all high-severity architecture review findings

This commit is contained in:
Joseph Doherty
2026-09-09 16:13:58 -04:00
parent 8bfe65512e
commit 146a13238f
192 changed files with 3160 additions and 753 deletions
@@ -0,0 +1,30 @@
using Google.Protobuf;
using Google.Protobuf.Collections;
using Grpc.Core;
namespace ZB.MOM.WW.ScadaBridge.Communication.Grpc;
/// <summary>Bounds audit batches using complete protobuf row sizes, length/tag prefixes, and gRPC framing.</summary>
public static class AuditWireBudget
{
/// <summary>Five-byte gRPC frame plus two bytes reserved for a true MoreAvailable field.</summary>
public const int InitialBytes = 7;
/// <summary>
/// Appends one row to a field-1 repeated-message collection if it fits. A row larger than the
/// normal budget travels alone, up to the hard channel ceiling. Oversize rows are retained
/// locally and fail explicitly with their identity; they are never acknowledged or truncated.
/// </summary>
public static bool TryAppend<T>(RepeatedField<T> rows, T row, ref int wireBytes, string rowIdentity)
where T : IMessage<T>
{
var rowBytes = 1L + CodedOutputStream.ComputeMessageSize(row);
if (rows.Count > 0 && wireBytes + rowBytes > GrpcMessageLimits.AuditBatchBytes) return false;
if (rowBytes + InitialBytes > GrpcMessageLimits.MaxMessageBytes)
throw new RpcException(new Status(StatusCode.ResourceExhausted,
$"Audit row {rowIdentity} requires {rowBytes + InitialBytes} wire bytes, exceeding {GrpcMessageLimits.MaxMessageBytes}; retained locally. Reduce its payload or raise coordinated peer limits before retrying."));
rows.Add(row);
wireBytes += (int)rowBytes;
return true;
}
}
@@ -117,7 +117,7 @@ public sealed class CentralChannelProvider : IDisposable
KeepAlivePingPolicy = HttpKeepAlivePingPolicy.Always,
EnableMultipleHttp2Connections = true,
},
}.WithSiteCredentials(pskProvider, siteId);
}.WithMessageLimits().WithSiteCredentials(pskProvider, siteId);
_channels[i] = GrpcChannel.ForAddress(endpoints[i], channelOptions);
_clients[i] = new CentralControlService.CentralControlServiceClient(_channels[i]);
@@ -56,8 +56,9 @@ namespace ZB.MOM.WW.ScadaBridge.Communication.Grpc;
/// to the peer central node only when the call provably never ran. So:
/// </para>
/// <list type="bullet">
/// <item><see cref="StatusCode.Unavailable"/> — this node is not ready; nothing was dispatched,
/// so a cross-node retry is safe and correct.</item>
/// <item><see cref="StatusCode.Unavailable"/> plus the server-owned not-started trailer — this
/// node is not ready; nothing was dispatched, so a cross-node retry is safe. Unavailable alone
/// can also be generated by a transport after application and does not prove retry safety.</item>
/// <item><see cref="StatusCode.DeadlineExceeded"/> — the <c>Ask</c> timed out. The message WAS
/// delivered and may have been processed; retrying it on the other node would duplicate work.</item>
/// <item><see cref="StatusCode.Internal"/> — the handler faulted (a piped
@@ -170,7 +171,7 @@ public sealed class CentralControlGrpcService : CentralControlService.CentralCon
var central = RequireReady(context);
var response = await AskAsync<NotificationStatusResponse>(
central,
CentralControlDtoMapper.FromDto(request),
CentralControlDtoMapper.FromDto(request) with { VerifiedSourceSiteId = CentralControlSiteIdentity.RequireVerified(context) },
_options.NotificationForwardTimeout,
context).ConfigureAwait(false);
@@ -332,9 +333,7 @@ public sealed class CentralControlGrpcService : CentralControlService.CentralCon
+ "the peer central node).",
context.Method);
throw new RpcException(new GrpcStatus(
StatusCode.Unavailable,
"Central control plane is not ready: the actor system is still starting."));
throw GrpcRetrySafety.NotDispatched("Central control plane is not ready: the actor system is still starting.");
}
/// <summary>
@@ -0,0 +1,50 @@
using Grpc.Core;
namespace ZB.MOM.WW.ScadaBridge.Communication.Grpc;
/// <summary>Verified site identity established by the central PSK interceptor, never by a request body.</summary>
public static class CentralControlSiteIdentity
{
private static readonly object IdentityKey = new();
/// <summary>Stores an identity only after the site's credential has been verified.</summary>
public static void SetVerified(ServerCallContext context, string siteId) => context.UserState[IdentityKey] = siteId;
/// <summary>Requires a credential-verified identity on the current call.</summary>
public static string RequireVerified(ServerCallContext context) =>
context.UserState.TryGetValue(IdentityKey, out var value) && value is string site && !string.IsNullOrWhiteSpace(site)
? site : throw Denied();
/// <summary>Checks every ownership field before dispatching any part of a central control request.</summary>
public static void ValidateRequest(object request, ServerCallContext context)
{
var site = RequireVerified(context);
void Owns(string? owner)
{
if (!string.Equals(site, owner, StringComparison.Ordinal)) throw Denied();
}
switch (request)
{
case NotificationSubmitDto n: Owns(n.SourceSiteId); break;
case ReconcileSiteRequestDto n: Owns(n.SiteIdentifier); break;
case SiteHealthReportDto n: Owns(n.SiteId); break;
case HeartbeatDto n: Owns(n.SiteId); break;
case AuditEventBatch batch:
foreach (var row in batch.Events) Owns(row.SourceSiteId);
break;
case CachedTelemetryBatch batch:
foreach (var packet in batch.Packets)
{
Owns(packet.AuditEvent?.SourceSiteId);
Owns(packet.Operational?.SourceSite);
}
break;
// The query carries no claimed owner. The service passes the verified identity to
// the outbox, which compares it with the stored notification's SourceSiteId.
case NotificationStatusQueryDto: break;
case Google.Protobuf.IMessage: throw Denied(); // new RPCs must explicitly choose an ownership rule
}
}
private static RpcException Denied() => new(new Status(StatusCode.PermissionDenied, "Control-plane site ownership denied."));
}
@@ -26,11 +26,10 @@ namespace ZB.MOM.WW.ScadaBridge.Communication.Grpc;
/// layers already treat as transient.
/// </para>
/// <para>
/// <b>Cross-node retry only on provably-unsent failures.</b> An <see cref="StatusCode.Unavailable"/>
/// (connection refused / node not ready) flips the channel pair and retries once on the peer.
/// A <see cref="StatusCode.DeadlineExceeded"/> is NEVER retried across nodes — a deploy / write /
/// failover may already have executed, and duplicating it is worse than surfacing a transient
/// failure the layer above tolerates.
/// <b>Cross-node retry is operation-specific.</b> Read-only status and idempotent notification,
/// audit and sequence-guarded health ingestion may retry transient transport failures. Reconcile
/// mints tokens and retries only an explicit server pre-dispatch rejection. Ambiguous reconcile
/// failures return OUTCOME_UNKNOWN. Both attempts share one absolute deadline.
/// </para>
/// <para>
/// <b>Per-call deadlines mirror today's Ask timeouts.</b> Notification submit/status →
@@ -71,7 +70,7 @@ public sealed class GrpcCentralTransport : ICentralTransport
var dto = CentralControlDtoMapper.ToDto(message);
Dispatch(replyTo, _options.NotificationForwardTimeout,
(c, o) => c.SubmitNotificationAsync(dto, o),
ack => CentralControlDtoMapper.FromDto(ack));
ack => CentralControlDtoMapper.FromDto(ack), retrySafe: true);
}
/// <inheritdoc />
@@ -80,7 +79,7 @@ public sealed class GrpcCentralTransport : ICentralTransport
var dto = CentralControlDtoMapper.ToDto(message);
Dispatch(replyTo, _options.NotificationForwardTimeout,
(c, o) => c.QueryNotificationStatusAsync(dto, o),
response => CentralControlDtoMapper.FromDto(response));
response => CentralControlDtoMapper.FromDto(response), retrySafe: true);
}
/// <inheritdoc />
@@ -89,7 +88,7 @@ public sealed class GrpcCentralTransport : ICentralTransport
var batch = CentralControlDtoMapper.ToDto(message);
Dispatch(replyTo, SiteStreamGrpcServer.AuditIngestAskTimeout,
(c, o) => c.IngestAuditEventsAsync(batch, o),
ack => new IngestAuditEventsReply(CentralControlDtoMapper.FromIngestAck(ack)));
ack => new IngestAuditEventsReply(CentralControlDtoMapper.FromIngestAck(ack)), retrySafe: true);
}
/// <inheritdoc />
@@ -98,7 +97,7 @@ public sealed class GrpcCentralTransport : ICentralTransport
var batch = CentralControlDtoMapper.ToDto(message);
Dispatch(replyTo, SiteStreamGrpcServer.AuditIngestAskTimeout,
(c, o) => c.IngestCachedTelemetryAsync(batch, o),
ack => new IngestCachedTelemetryReply(CentralControlDtoMapper.FromIngestAck(ack)));
ack => new IngestCachedTelemetryReply(CentralControlDtoMapper.FromIngestAck(ack)), retrySafe: true);
}
/// <inheritdoc />
@@ -116,7 +115,7 @@ public sealed class GrpcCentralTransport : ICentralTransport
var dto = CentralControlDtoMapper.ToDto(message);
Dispatch(replyTo, _options.HealthReportTimeout,
(c, o) => c.ReportSiteHealthAsync(dto, o),
ack => CentralControlDtoMapper.FromDto(ack));
ack => CentralControlDtoMapper.FromDto(ack), retrySafe: true);
}
/// <inheritdoc />
@@ -156,34 +155,39 @@ public sealed class GrpcCentralTransport : ICentralTransport
IActorRef replyTo,
TimeSpan timeout,
Func<CentralControlService.CentralControlServiceClient, CallOptions, AsyncUnaryCall<TWire>> call,
Func<TWire, object> decode)
Func<TWire, object> decode, bool retrySafe = false)
{
_ = DispatchAsync(replyTo, timeout, call, decode);
_ = DispatchAsync(replyTo, timeout, call, decode, retrySafe);
}
private async Task DispatchAsync<TWire>(
IActorRef replyTo,
TimeSpan timeout,
Func<CentralControlService.CentralControlServiceClient, CallOptions, AsyncUnaryCall<TWire>> call,
Func<TWire, object> decode)
Func<TWire, object> decode, bool retrySafe = false)
{
var (index, client) = _channels.Current();
var deadline = DateTime.UtcNow.Add(timeout);
try
{
var reply = await InvokeAsync(client, timeout, call).ConfigureAwait(false);
var reply = await InvokeAsync(client, deadline, call).ConfigureAwait(false);
replyTo.Tell(decode(reply));
}
catch (RpcException ex) when (IsConnectFailure(ex))
{
// Provably unsent: the connection was refused / the node was not ready. Fail over
// to the peer and retry ONCE. This is the only status we retry across nodes.
_channels.ReportUnavailable(index);
if (!retrySafe && !GrpcRetrySafety.WasNotDispatched(ex))
{
replyTo.Tell(new AkkaStatus.Failure(GrpcRetrySafety.OutcomeUnknown(ex)));
return;
}
// Retry only a read/idempotent ingest or an explicit server pre-dispatch rejection.
var (retryIndex, retryClient) = _channels.Current();
if (retryIndex != index)
{
try
{
var reply = await InvokeAsync(retryClient, timeout, call).ConfigureAwait(false);
var reply = await InvokeAsync(retryClient, deadline, call).ConfigureAwait(false);
replyTo.Tell(decode(reply));
return;
}
@@ -191,7 +195,10 @@ public sealed class GrpcCentralTransport : ICentralTransport
{
_logger.LogWarning(retryEx,
"Central control-plane call failed on both endpoints; surfacing as transient.");
replyTo.Tell(new AkkaStatus.Failure(retryEx));
var reported = retryEx is RpcException rpc && IsConnectFailure(rpc)
&& !retrySafe && !GrpcRetrySafety.WasNotDispatched(rpc)
? GrpcRetrySafety.OutcomeUnknown(rpc) : retryEx;
replyTo.Tell(new AkkaStatus.Failure(reported));
return;
}
}
@@ -209,27 +216,18 @@ public sealed class GrpcCentralTransport : ICentralTransport
private static async Task<TWire> InvokeAsync<TWire>(
CentralControlService.CentralControlServiceClient client,
TimeSpan timeout,
DateTime deadline,
Func<CentralControlService.CentralControlServiceClient, CallOptions, AsyncUnaryCall<TWire>> call)
{
var options = new CallOptions(deadline: DateTime.UtcNow.Add(timeout));
var options = new CallOptions(deadline: deadline);
using var asyncCall = call(client, options);
return await asyncCall.ResponseAsync.ConfigureAwait(false);
}
/// <summary>
/// A failure that provably never reached a server — the only class safe to retry on the peer.
/// <see cref="StatusCode.DeadlineExceeded"/> is deliberately excluded (the call may have run).
/// Identifies transient connection failures for endpoint selection. This does NOT prove
/// that a request was unsent; the caller separately checks operation replay safety.
/// </summary>
/// <remarks>
/// Two shapes qualify: a server-signalled <see cref="StatusCode.Unavailable"/> (e.g. a node
/// that returns Unavailable while it is still starting), and a client-side failure to even
/// start the call — Grpc.Net surfaces a refused/failed connection as
/// <see cref="StatusCode.Internal"/> "Error starting gRPC call" with the transport exception
/// attached, and there the request never left the client. Anything else — including a deadline,
/// a permission denial, or a generic server-side Internal after the call reached the server —
/// is NOT retried across nodes.
/// </remarks>
private static bool IsConnectFailure(RpcException ex)
{
if (ex.StatusCode == StatusCode.Unavailable)
@@ -237,8 +235,8 @@ public sealed class GrpcCentralTransport : ICentralTransport
return true;
}
// "Error starting gRPC call" is Grpc.Net's marker for a call that could not be sent — a
// refused/failed connection carrying an HttpRequestException. Provably unsent.
// A transport-originated Internal can also select a new endpoint; it provides no
// permission to repeat a non-idempotent operation.
return ex.StatusCode == StatusCode.Internal
&& (ex.Status.DebugException is HttpRequestException
|| ex.Status.Detail.StartsWith("Error starting gRPC call", StringComparison.Ordinal));
@@ -0,0 +1,23 @@
using Grpc.Net.Client;
namespace ZB.MOM.WW.ScadaBridge.Communication.Grpc;
/// <summary>Shared wire ceilings for both ends of the control plane.</summary>
public static class GrpcMessageLimits
{
/// <summary>Maximum serialized message size, including all fields and protobuf framing.</summary>
public const int MaxMessageBytes = 32 * 1024 * 1024;
/// <summary>Normal audit batch budget, leaving ample room below the wire ceiling.</summary>
public const int AuditBatchBytes = 2 * 1024 * 1024;
/// <summary>Applies the control-plane limits to a client channel.</summary>
/// <param name="options">Channel options, including the caller's handler and credentials.</param>
/// <returns>The supplied options for chaining.</returns>
public static GrpcChannelOptions WithMessageLimits(this GrpcChannelOptions options)
{
options.MaxReceiveMessageSize = MaxMessageBytes;
options.MaxSendMessageSize = MaxMessageBytes;
return options;
}
}
@@ -0,0 +1,23 @@
using Grpc.Core;
namespace ZB.MOM.WW.ScadaBridge.Communication.Grpc;
/// <summary>Separates an explicit server rejection before dispatch from a lost response after possible application.</summary>
public static class GrpcRetrySafety
{
/// <summary>Server-owned trailer emitted only before any actor dispatch.</summary>
public const string DispatchTrailer = "scadabridge-dispatch";
/// <summary>Creates a readiness rejection that proves no work was dispatched.</summary>
public static RpcException NotDispatched(string detail) => new(
new Status(StatusCode.Unavailable, detail), new Metadata { { DispatchTrailer, "not-started" } });
/// <summary>True only for an explicit rejection from the server before it dispatched work.</summary>
public static bool WasNotDispatched(RpcException error) => error.StatusCode == StatusCode.Unavailable
&& error.Trailers.Any(e => e.Key == DispatchTrailer && e.Value == "not-started");
/// <summary>Reports an ambiguous mutation without inviting a blind replay.</summary>
public static RpcException OutcomeUnknown(RpcException error) => new(
new Status(StatusCode.Aborted, "OUTCOME_UNKNOWN: connection failed after the command may have been applied. Verify its state before retrying."),
error.Trailers);
}
@@ -28,8 +28,9 @@ namespace ZB.MOM.WW.ScadaBridge.Communication.Grpc;
/// path, which the S&amp;F/audit layers already treat as transient.
/// </para>
/// <para>
/// <b>Cross-node retry is the channel provider's job</b> and happens only on
/// <c>Unavailable</c> — never on <c>DeadlineExceeded</c> (a write/deploy/failover may have run).
/// <b>Cross-node retry is operation-specific.</b> Read-only commands can retry Unavailable;
/// mutations require an explicit server rejection before dispatch. A lost mutation response
/// returns OUTCOME_UNKNOWN, never an automatic duplicate. Deadlines are not replayed.
/// </para>
/// </remarks>
public sealed class GrpcSiteTransport : ISiteCommandTransport
@@ -125,9 +126,15 @@ public sealed class GrpcSiteTransport : ISiteCommandTransport
return _channels.ExecuteAsync(
envelope.SiteId,
(channel, callCt) => InvokeAsync(channel, group, message, deadline, callCt),
ct);
ct, retrySafe: IsRetrySafe(message));
}
/// <summary>Only read-only commands can replay after a response may have been lost.</summary>
internal static bool IsRetrySafe(object message) => message is
DeploymentStateQueryRequest or BrowseNodeCommand or SearchAddressSpaceCommand or ReadTagValuesCommand
or ListServerCertsCommand or EventLogQueryRequest or DebugSnapshotRequest or ParkedMessageQueryRequest
or RouteToGetAttributesRequest;
private static async Task<object> InvokeAsync(
GrpcChannel channel, SiteCommandGroup group, object message, DateTime deadline, CancellationToken ct)
{
@@ -196,8 +196,7 @@ public sealed class SiteCommandGrpcService : SiteCommandService.SiteCommandServi
var dispatcher = _dispatcher;
if (!_ready || dispatcher is null)
{
throw new RpcException(new GrpcStatus(
StatusCode.Unavailable, "Site command plane not ready."));
throw GrpcRetrySafety.NotDispatched("Site command plane not ready.");
}
return dispatcher;
}
@@ -31,13 +31,10 @@ public sealed class SiteChannelUnavailableException(string siteId)
/// </summary>
/// <remarks>
/// <para>
/// <b>Retry safety.</b> <see cref="ExecuteAsync{T}"/> retries on the other node ONLY when the first
/// attempt failed with <see cref="StatusCode.Unavailable"/> (provably never reached a server —
/// connect refused or readiness-rejected before response headers). A
/// <see cref="StatusCode.DeadlineExceeded"/>, or any other status, is rethrown WITHOUT a cross-node
/// retry: a <c>WriteTag</c>/<c>DeployArtifacts</c>/<c>TriggerSiteFailover</c> may already have
/// executed, and the layers above tolerate the one-shot ambiguity exactly as ClusterClient's Ask
/// did — the migration must not turn one write into two.
/// <b>Retry safety.</b> Unavailable can mean a lost response after application. Only explicitly
/// retry-safe operations or a server-owned not-started trailer permit replay on the peer.
/// Ambiguous mutations fail with Aborted / OUTCOME_UNKNOWN; the next independent call uses the
/// peer. Deadlines are never replayed. Readiness rejection remains safe before actor dispatch.
/// </para>
/// <para>
/// <b>Addresses.</b> Fed by <see cref="UpdateSite"/>/<see cref="RemoveSite"/> from the single DB
@@ -188,17 +185,18 @@ public sealed class SitePairChannelProvider : IDisposable
/// <summary>
/// Runs <paramref name="call"/> against the site's current sticky channel, failing over to the
/// other node once on <see cref="StatusCode.Unavailable"/> (never on
/// <see cref="StatusCode.DeadlineExceeded"/>). Ensures the background failback loop is running.
/// other node only for a retry-safe operation or proven pre-dispatch rejection. An ambiguous
/// mutation returns OUTCOME_UNKNOWN. Ensures the background failback loop is running.
/// </summary>
/// <typeparam name="T">The RPC reply type.</typeparam>
/// <param name="siteId">Target site.</param>
/// <param name="call">The RPC to run against a given channel.</param>
/// <param name="ct">Cancellation token.</param>
/// <param name="retrySafe">Whether repeating this operation is safe after an ambiguous transport failure.</param>
/// <returns>The RPC reply.</returns>
/// <exception cref="SiteChannelUnavailableException">The site has no configured channel.</exception>
public async Task<T> ExecuteAsync<T>(
string siteId, Func<GrpcChannel, CancellationToken, Task<T>> call, CancellationToken ct)
string siteId, Func<GrpcChannel, CancellationToken, Task<T>> call, CancellationToken ct, bool retrySafe = false)
{
ArgumentException.ThrowIfNullOrWhiteSpace(siteId);
ArgumentNullException.ThrowIfNull(call);
@@ -227,14 +225,22 @@ public sealed class SitePairChannelProvider : IDisposable
{
return await call(primary, ct).ConfigureAwait(false);
}
catch (RpcException ex) when (ex.StatusCode == StatusCode.Unavailable && secondary is not null)
catch (RpcException ex) when (ex.StatusCode == StatusCode.Unavailable)
{
// Provably never reached a server → safe to try the other node, even for a write.
if (secondary is not null) FlipTo(pair, toIsA: !primaryIsA);
if (!retrySafe && !GrpcRetrySafety.WasNotDispatched(ex))
throw GrpcRetrySafety.OutcomeUnknown(ex);
if (secondary is null) throw;
// Only reads/idempotent operations or an explicit pre-dispatch rejection may replay.
_logger.LogWarning(
"Site {SiteId} node {FromNode} unavailable; failing over to node {ToNode}",
siteId, primaryIsA ? "A" : "B", primaryIsA ? "B" : "A");
FlipTo(pair, toIsA: !primaryIsA);
return await call(secondary, ct).ConfigureAwait(false);
try { return await call(secondary, ct).ConfigureAwait(false); }
catch (RpcException retryError) when (retryError.StatusCode == StatusCode.Unavailable
&& !retrySafe && !GrpcRetrySafety.WasNotDispatched(retryError))
{
throw GrpcRetrySafety.OutcomeUnknown(retryError);
}
}
}
@@ -432,7 +438,7 @@ public sealed class SitePairChannelProvider : IDisposable
KeepAlivePingPolicy = HttpKeepAlivePingPolicy.Always,
EnableMultipleHttp2Connections = true
}
}.WithSiteCredentials(_pskProvider, siteId);
}.WithMessageLimits().WithSiteCredentials(_pskProvider, siteId);
return GrpcChannel.ForAddress(endpoint, channelOptions);
}
@@ -93,7 +93,7 @@ public class SiteStreamGrpcClient : IAsyncDisposable, IDisposable
KeepAlivePingTimeout = options.GrpcKeepAlivePingTimeout,
KeepAlivePingPolicy = HttpKeepAlivePingPolicy.Always
}
}.WithSiteCredentials(pskProvider, siteIdentifier));
}.WithMessageLimits().WithSiteCredentials(pskProvider, siteIdentifier));
_client = new SiteStreamService.SiteStreamServiceClient(_channel);
_logger = logger;
}
@@ -792,9 +792,14 @@ public class SiteStreamGrpcServer : SiteStreamService.SiteStreamServiceBase
// (plus, once it sets after_id, that row's EventId).
MoreAvailable = events.Count >= request.BatchSize,
};
var wireBytes = AuditWireBudget.InitialBytes;
foreach (var evt in events)
{
response.Events.Add(AuditEventDtoMapper.ToDto(evt));
if (!AuditWireBudget.TryAppend(response.Events, AuditEventDtoMapper.ToDto(evt), ref wireBytes, evt.EventId.ToString()))
{
response.MoreAvailable = true;
break;
}
}
return response;
@@ -860,9 +865,14 @@ public class SiteStreamGrpcServer : SiteStreamService.SiteStreamServiceBase
// is upsert-on-newer, so re-reading rows is a harmless no-op.
MoreAvailable = operationals.Count >= request.BatchSize,
};
var wireBytes = AuditWireBudget.InitialBytes;
foreach (var op in operationals)
{
response.Operationals.Add(SiteCallDtoMapper.ToDto(op));
if (!AuditWireBudget.TryAppend(response.Operationals, SiteCallDtoMapper.ToDto(op), ref wireBytes, op.TrackedOperationId.ToString()))
{
response.MoreAvailable = true;
break;
}
}
return response;