test(mesh): close review findings on the PreStart-race logging fix — non-vacuous once-per-incarnation pin, broken-dialer symmetry, comment idiom

Follow-up to a94eb8a3, addressing CHANGES REQUESTED on code review. Production logic was confirmed correct on all spec points; this commit fixes the review's test/comment findings only. IMPORTANT-1: A_permanently_unresolvable_pskProvider_is_reported_once_per_incarnation_not_once_per_refresh did not prove the second refresh ran — its Expect(0) window gated on a dialer dispatch landing, but LoadContactsFromDb is fire-and-forget and the first block's AwaitCondition already re-Tells MeshCommand per poll, so the dialer call count could reach the threshold before PublishKnownClusters was ever re-invoked by the second load. Fixed by having the PSK factory count its own invocations and gating both AwaitConditions on that count directly (pskCalls >= 2 proves the first retry; pskCalls > pskCallsAfterFirstLoad, awaited INSIDE the Expect(0) window, proves the second one). Bite-checked: with _pskProviderAbsenceReported's guard temporarily removed, the test now correctly fails ("Received 1 message too many... received 1 that matched filter"); guard restored and reverified green. MINOR-2: reworded four comments (CentralCommunicationActor.cs ~109, ~259, ~305-309, ~853) from narrating the change ("this fix removes", "already existed, unchanged", "added by this fix", "the old two-call-site shape") to stating the timeless invariant. MINOR-3: added A_supplied_dialer_factory_that_always_throws_still_drops_each_command_loudly_while_PreStart_stays_quiet — the symmetric case to the existing never-supplied-dialer test, exercising Resolve's catch + firstUse:true path (a factory that WAS supplied and always throws) rather than the null-factory path. Asserts PreStart plus the first contact load stay silent and DispatchOverGrpc's existing per-dispatch Error still fires on every dispatch, unguarded. MINOR-4: "Check the ControlPlane registration wires IMeshPskProvider" -> "Check that the ControlPlane registration wires up IMeshPskProvider"; no test pinned the old substring, so no test update was needed for this one. Skipped item 5 (restart-re-arms pin) per instruction — out of scope. Claude-Session: https://claude.ai/code/session_014WNM4vjoVksyyBraTXSZE1
2026-08-06 10:30:20 -04:00
parent 2e06dfe52d
commit d7c9ead64e
2 changed files with 87 additions and 41 deletions
@@ -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 —
-    ///         <see cref="DispatchOverGrpc"/> for the dialer (already existed, unchanged) and
-    ///         <see cref="PublishKnownClusters"/> for the PSK provider (added by this fix, guarded
-    ///         to once per incarnation because that call site runs on a timer forever, where
-    ///         <see cref="DispatchOverGrpc"/> only runs once per command).
+    ///         <see cref="DispatchOverGrpc"/> for the dialer (fires every dropped command) and
+    ///         <see cref="PublishKnownClusters"/> for the PSK provider (fires once per incarnation,
+    ///         because that call site runs on a timer forever, where <see cref="DispatchOverGrpc"/>
+    ///         only runs once per command).
    ///     </para>
    /// </remarks>
    private T? Resolve<T>(Func<T?>? factory, string name, bool firstUse)
@@ -854,13 +851,13 @@ public sealed class CentralCommunicationActor : ReceiveActor, IWithTimers
    ///     </para>
    ///     <para>
    ///         <b>This is the PSK provider's "first use" call site.</b> <see cref="PskProvider"/> 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 <c>ContactRefreshSeconds</c> 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 <c>ContactRefreshSeconds</c> forever.
    ///     </para>
    /// </remarks>
    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)");
            }

@@ -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<OtOpcUaConfigDbContext> factory, string nodeId, int? grpcPort)
    {