diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Communication/CentralCommunicationActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Communication/CentralCommunicationActor.cs index b72c44ac..316a1c4a 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Communication/CentralCommunicationActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Communication/CentralCommunicationActor.cs @@ -104,10 +104,9 @@ public sealed class CentralCommunicationActor : ReceiveActor, IWithTimers // Set once PublishKnownClusters has reported a genuinely-absent PSK provider (a factory WAS // supplied and still resolves null after the first-use retry). PublishKnownClusters runs on - // the contact-refresh timer forever, so without this an actor with a permanently broken - // provider would repeat the Error every ContactRefreshSeconds for its whole life — the same - // per-refresh noise this fix removes, just relocated. Same warn-once shape as - // _warnedPortlessNodes; actor thread only. + // the contact-refresh timer for the actor's whole life, so an unguarded Error there would + // repeat every ContactRefreshSeconds forever. Same warn-once shape as _warnedPortlessNodes; + // actor thread only. private bool _pskProviderAbsenceReported; // The gRPC dispatch set, replaced wholesale on every contact load. Actor thread only; the fan @@ -257,14 +256,12 @@ public sealed class CentralCommunicationActor : ReceiveActor, IWithTimers } // Deliberately NO null guard for either dependency here. At this point a still-null value - // is indistinguishable from the starter-ordering race Resolve just logged quietly above — - // reporting it loudly here would be the exact false alarm this fix removes, on nearly - // every boot. A retry that ALSO fails — the genuinely permanent absence — is reported - // loudly where it actually costs something instead: DispatchOverGrpc for the dialer (every - // dropped command, unchanged — see the EventFilter pin on that message), and - // PublishKnownClusters for the PSK provider (once per incarnation, not once per refresh - // forever, since that handler runs on the contact-refresh timer for the actor's whole - // life). + // is indistinguishable from the starter-ordering race Resolve just logged quietly above, + // so a loud report here cannot tell a lost race from a genuine absence — that decision + // belongs where a still-null result after the memoized properties' first-use retry + // actually costs something: DispatchOverGrpc for the dialer (every dropped command), and + // PublishKnownClusters for the PSK provider (once per incarnation, since that handler runs + // on the contact-refresh timer for the actor's whole life). // Started in EVERY mode. The refresh is not transport state: it is central's ClusterNode // inventory, and the known-cluster gate it feeds protects an endpoint mapped by PORT, not by @@ -308,10 +305,10 @@ public sealed class CentralCommunicationActor : ReceiveActor, IWithTimers /// only the caller knows whether a value came back after the retry the memoized /// properties perform. So the loud, operator-facing signal is deliberately NOT here: it /// is at the two places that consume a still-null result after that retry — - /// for the dialer (already existed, unchanged) and - /// for the PSK provider (added by this fix, guarded - /// to once per incarnation because that call site runs on a timer forever, where - /// only runs once per command). + /// for the dialer (fires every dropped command) and + /// for the PSK provider (fires once per incarnation, + /// because that call site runs on a timer forever, where + /// only runs once per command). /// /// private T? Resolve(Func? factory, string name, bool firstUse) @@ -854,13 +851,13 @@ public sealed class CentralCommunicationActor : ReceiveActor, IWithTimers /// /// /// This is the PSK provider's "first use" call site. is - /// evaluated exactly ONCE per call (not once per statement — the old two-call-site shape - /// doubled the memoized property's retry rate for no benefit) and its result reused for - /// both calls below. A still-null result here means the property's own retry, on top of - /// PreStart's original attempt, has ALSO failed — the genuinely permanent absence — so - /// this is where that gets reported loudly, guarded to once per incarnation because this - /// handler runs on the contact-refresh timer for the actor's entire life and an unguarded - /// Error here would repeat every ContactRefreshSeconds forever. + /// evaluated exactly once per call and its result reused below; evaluating it + /// per-statement would double the memoized property's retry rate for no benefit. A + /// still-null result here means the property's own retry, on top of PreStart's original + /// attempt, has ALSO failed — the genuinely permanent absence — so this is where that + /// gets reported loudly, guarded to once per incarnation because this handler runs on + /// the contact-refresh timer for the actor's entire life and an unguarded Error here + /// would repeat every ContactRefreshSeconds forever. /// /// private void PublishKnownClusters(ContactsLoaded msg) @@ -879,7 +876,7 @@ public sealed class CentralCommunicationActor : ReceiveActor, IWithTimers + "the known-cluster gate will NEVER be published and cached per-cluster keys " + "will NEVER be dropped. Inbound mesh acks stay exposed to the " + "pre-authentication secret-store cost the gate removes, and a rotated key " - + "needs a central restart. Check the ControlPlane registration wires " + + "needs a central restart. Check that the ControlPlane registration wires up " + "IMeshPskProvider (this will not be logged again this incarnation)"); } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/Communication/CentralCommunicationActorTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/Communication/CentralCommunicationActorTests.cs index 423e0b3d..4a0bdc57 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/Communication/CentralCommunicationActorTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/Communication/CentralCommunicationActorTests.cs @@ -902,9 +902,21 @@ public class CentralCommunicationActorTests : ControlPlaneActorTestBase // must still be loud — but PublishKnownClusters runs on the contact-refresh timer for the // actor's whole life, so an unguarded Error would repeat every ContactRefreshSeconds forever. // Guarded exactly like _warnedPortlessNodes: once per incarnation, not once per refresh. + // + // The factory counts its own invocations rather than relying on a dialer/dispatch side + // effect: LoadContactsFromDb is fire-and-forget, so a signal that isn't the resolve itself + // (e.g. a dispatched command landing) can close an AwaitCondition window before the SECOND + // contact load has actually re-invoked PublishKnownClusters, making the "does not repeat" + // half true for the wrong reason. var factory = NewInMemoryDbFactory(); SeedGrpcCluster(factory, "C1", ("host-a:4053", "host-a", 4056, true, false)); - var dialer = new RecordingDialer(); + var pskCalls = 0; + + IMeshPskProvider? PskFactory() + { + pskCalls++; + throw new InvalidOperationException("permanently broken"); + } IActorRef actor = null!; EventFilter.Error(contains: "will NEVER be published").ExpectOne(TimeSpan.FromSeconds(5), () => @@ -914,31 +926,68 @@ public class CentralCommunicationActorTests : ControlPlaneActorTestBase GrpcMode(), new RecordingClientFactory(() => CreateTestProbe()), () => null, - () => dialer, - pskProvider: () => throw new InvalidOperationException("permanently broken"))); + () => new RecordingDialer(), + pskProvider: PskFactory)); - // Proof the FIRST contact load actually completed (the gRPC target set is adopted - // before the gate publish this Error guards — see HandleContactsLoaded), rather than - // relying on timing alone. - AwaitCondition(() => - { - actor.Tell(new MeshCommand(DeploymentsTopic, NewDispatch())); - return dialer.Calls.Count >= 1; - }, TimeSpan.FromSeconds(5)); + // pskCalls == 1 is PreStart's own attempt; reaching 2 is PROOF the first contact load + // ran and PublishKnownClusters's property access performed the first-use retry — not + // merely that the actor was constructed. + AwaitCondition(() => pskCalls >= 2, TimeSpan.FromSeconds(5)); }); - // A SECOND contact load must NOT repeat it. + var pskCallsAfterFirstLoad = pskCalls; + + // A SECOND contact load must NOT repeat the Error. The pin only means something if that + // second load actually happened and retried the resolve again — AwaitCondition on a + // STRICTLY higher pskCalls count, taken INSIDE the very block whose zero-Error assertion + // depends on it, is what proves that rather than assuming it from elapsed time. EventFilter.Error(contains: "will NEVER be published").Expect(0, TimeSpan.FromSeconds(5), () => { actor.Tell(new CentralCommunicationActor.RefreshContacts()); - AwaitCondition(() => - { - actor.Tell(new MeshCommand(DeploymentsTopic, NewDispatch())); - return dialer.Calls.Count >= 2; - }, TimeSpan.FromSeconds(5)); + AwaitCondition(() => pskCalls > pskCallsAfterFirstLoad, TimeSpan.FromSeconds(5)); }); } + [Fact] + public void A_supplied_dialer_factory_that_always_throws_still_drops_each_command_loudly_while_PreStart_stays_quiet() + { + // The symmetric case to Grpc_mode_with_no_dialer_wired_drops_the_command_with_an_error + // above: there the factory was never supplied (Resolve returns null with no exception, + // silent at every call site); here a factory WAS supplied and never succeeds, so it goes + // through Resolve's catch on both the PreStart attempt (firstUse: false, Debug) and every + // first-use retry (firstUse: true, still Debug — see Resolve's remarks on why severity is + // decided by the consumer, not by Resolve). The loud signal is DispatchOverGrpc's existing + // per-dispatch Error, unaffected by an exception being the reason the dialer is null rather + // than a bare absence, and unguarded — unlike the PSK provider, every dispatch reports it. + var factory = NewInMemoryDbFactory(); + SeedGrpcCluster(factory, "C1", ("host-a:4053", "host-a", 4056, true, false)); + var psk = new RecordingPskProvider(); + + IActorRef actor = null!; + EventFilter.Warning().And.Error().Expect(0, TimeSpan.FromSeconds(5), () => + { + actor = Sys.ActorOf(CentralCommunicationActor.Props( + factory, + GrpcMode(), + new RecordingClientFactory(() => CreateTestProbe()), + () => null, + dialer: () => throw new InvalidOperationException("permanently broken"), + pskProvider: () => psk)); + + // Proof PreStart's resolve attempt and the first contact load it schedules have + // actually run, so this block's silence is not vacuous. + AwaitCondition(() => psk.KnownClusterPublications.Count >= 1, TimeSpan.FromSeconds(5)); + }); + + EventFilter.Error(contains: "no command dialer is wired").ExpectOne(TimeSpan.FromSeconds(5), () => + actor.Tell(new MeshCommand(DeploymentsTopic, NewDispatch()))); + + // A SECOND dispatch must ALSO log it: this signal is per-dispatch and unguarded, the + // opposite shape from the PSK provider's once-per-incarnation guard above. + EventFilter.Error(contains: "no command dialer is wired").ExpectOne(TimeSpan.FromSeconds(5), () => + actor.Tell(new MeshCommand(DeploymentsTopic, NewDispatch()))); + } + private static void SetGrpcPort( IDbContextFactory factory, string nodeId, int? grpcPort) {