fix(mesh): 1B.1 review follow-ups

Two review items from Task 1B.1, both diagnostic:

1. CentralCommandGrpcService's "actor not ready" Warning omitted the peer and
   the correlation id while the decode-failure Warning six lines above carried
   both. These are the only two ways an ack fails to reach the coordinator, and
   an operator has to correlate either against the node's own log. Decoding has
   already succeeded on this path, so the DECODED correlation id is free.

2. A fused driver,admin,cluster-MAIN node under Mode=Grpc owes BOTH validator
   surfaces at once (driver keys AND ConfigServe:GrpcListenPort). The two blocks
   are independent `if`s; chained as `else if` a fused central node would be
   graded on one surface only and boot with nowhere for any ack to land. Pinned
   from both sides plus the fully-configured pass.
This commit is contained in:
Joseph Doherty
2026-08-06 05:36:16 -04:00
parent 1368578435
commit e1db90f9c9
3 changed files with 53 additions and 4 deletions
@@ -97,11 +97,17 @@ public sealed class CentralCommandGrpcService : GeneratedServiceBase
if (!_registry.TryGet<CentralCommunicationActorKey>(out var actor))
{
// Peer and correlation id are carried here for the same reason the decode-failure warning
// above carries them: these two rejections are the only ways an ack fails to reach the
// coordinator, and an operator has to correlate either against the node's own log. The
// decode has already succeeded on this path, so the DECODED correlation id is used —
// unlike above, where the raw wire string is the only value guaranteed readable.
_log.LogWarning(
"Received an ApplyAck for {DeploymentId} from {NodeId} before the central "
+ "communication actor was registered on this node; refusing with Unavailable so the "
+ "caller retries rather than recording a failure against a node that applied",
ack.DeploymentId, ack.NodeId);
"Received an ApplyAck for {DeploymentId} (correlation '{CorrelationId}') from {NodeId} "
+ "at {Peer} before the central communication actor was registered on this node; "
+ "refusing with Unavailable so the caller retries rather than recording a failure "
+ "against a node that applied",
ack.DeploymentId, ack.CorrelationId, ack.NodeId, context.Peer);
throw new RpcException(new Status(StatusCode.Unavailable, "central command actor not ready"));
}
@@ -454,6 +454,29 @@ public class MeshTransportOptionsValidatorTests
"driver", "cluster-SITE-A").Succeeded.ShouldBeTrue();
}
[Fact]
public void Grpc_fused_admin_and_driver_node_must_satisfy_BOTH_surfaces()
{
// The central pair is fused (admin,driver,cluster-MAIN), so it is the one node shape that owes
// both halves at once — and the two blocks are independent `if`s, not an `if`/`else if`. Were
// they chained, a fused node would be graded on the driver surface only and its missing
// ConfigServe listener would pass validation, leaving central with no place for any node's ack
// to land. Asserted from BOTH sides so a future chaining in either order is caught.
var missingNodeSide = new MeshTransportOptions { Mode = MeshTransportOptions.ModeGrpc };
var withPort = Validate(missingNodeSide, ConfigServePort("4055"), "driver", "admin", "cluster-MAIN");
withPort.Failed.ShouldBeTrue();
withPort.FailureMessage.ShouldContain("MeshTransport:GrpcPsk");
withPort.FailureMessage.ShouldContain("MeshTransport:CentralGrpcEndpoints");
var missingAdminSide = Validate(WellFormedGrpc(), "driver", "admin", "cluster-MAIN");
missingAdminSide.Failed.ShouldBeTrue();
missingAdminSide.FailureMessage.ShouldContain("ConfigServe:GrpcListenPort");
// And the fully-configured fused node passes — the rule is "both", not "neither".
Validate(WellFormedGrpc(), ConfigServePort("4055"), "driver", "admin", "cluster-MAIN")
.Succeeded.ShouldBeTrue();
}
[Fact]
public void ClusterPsks_defaults_to_an_empty_case_insensitive_map()
{
@@ -182,6 +182,26 @@ public sealed class CentralCommandGrpcServiceTests : TestKit
.ShouldBeTrue("the rejection log must carry the raw correlation id");
}
[Fact]
public async Task A_not_ready_rejection_logs_the_peer_and_the_correlation_id()
{
// Diagnostic symmetry with the decode-failure warning above. The two rejections are the only
// two ways an ack fails to reach the coordinator, and an operator reading the central log has
// to correlate BOTH against the node's own log — which needs the correlation id — and against
// which node was calling, which needs the peer. Decoding has already succeeded on this path,
// so the decoded correlation id costs nothing to include.
var logger = new CapturingLogger<CentralCommandGrpcService>();
var request = WellFormed();
await Should.ThrowAsync<RpcException>(() =>
new CentralCommandGrpcService(new ActorRegistry(), logger)
.ReportApply(request, new FakeServerCallContext()));
var warning = logger.Entries.Single(e => e.Level == LogLevel.Warning);
warning.Message.ShouldContain("ipv4:127.0.0.1:0");
warning.Message.ShouldContain(request.CorrelationId);
}
/// <summary>Test logger that captures all log entries.</summary>
/// <typeparam name="T">The category type.</typeparam>
private sealed class CapturingLogger<T> : ILogger<T>