fix(#654): review fixes - non-vacuous stop-on-publish test, Error only when retry budget refuses

Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe
2026-10-08 23:39:06 -04:00
parent 9dc6390d50
commit 2659a690a6
3 changed files with 30 additions and 17 deletions
@@ -171,7 +171,7 @@ The checked-in `appsettings*.json` files are deliberately thin: they carry only
 | Key | Type | Default | Meaning |
 |---|---|---|---|
 | `Mode` | string | `Dps` | `Dps` = DistributedPubSub, pair-local only — cannot cross an application-`Cluster` boundary. `Grpc` = the `mesh_transport.v1` gRPC contract (below), required on any node carrying a `cluster-{ClusterId}` role. Any other value **refuses to start**, including the retired `ClusterClient` value — a node still configured for it is refused at boot rather than silently demoted to `Dps`, which on a split-topology node would deliver nothing at all. |
-| `ContactRefreshSeconds` | int | `60` | Central-side only: how often central rebuilds its node dispatch set — the gRPC dial targets and the known-cluster PSK gate — from the `ClusterNode` rows. Runs in every mode; under `Dps` it still keeps the inbound-ack endpoint's known-cluster gate current even though nothing dials out. While the known-cluster gate is still unpublished (PSK provider not yet resolvable at boot, or the first `ClusterNode` load failed) central also retries a refresh every 2 s, at most 15 times, until the first publish (#654); steady state adds no traffic. |
+| `ContactRefreshSeconds` | int | `60` | Central-side only: how often central rebuilds its node dispatch set — the gRPC dial targets and the known-cluster PSK gate — from the `ClusterNode` rows. Runs in every mode; under `Dps` it still keeps the inbound-ack endpoint's known-cluster gate current even though nothing dials out. While the known-cluster gate is still unpublished (PSK provider not yet resolvable at boot, or the first `ClusterNode` load failed) central also retries a refresh every 2 s, at most 15 times, until the first publish (#654); steady state adds no traffic. Load failures and an unresolvable provider share the one 15-retry (~30 s) budget; after it is spent, healing falls to the periodic refresh. |

 **`Grpc` mode: central dials each node, a node posts its ack to central.** `CentralCommunicationActor` fans one `NodeCommandService.Execute` call per node — the target set is every enabled, non-maintenance `ClusterNode` row that carries a `GrpcPort` (dialled at `Host`+`GrpcPort`), rebuilt every `ContactRefreshSeconds`. There is no receptionist and no single fleet-wide (or per-cluster) client object: it is a plain per-node dial. A node posts its `ApplyAck` with `GrpcApplyAckSender`, which walks `CentralGrpcEndpoints` in order and stops at the first success — an availability list, not a fan-out list, so central normally sees one ack per node per dispatch (the walk as a whole is at-least-once on a slow-then-retried endpoint; `ConfigPublishCoordinator.HandleAck` absorbs a duplicate idempotently). Both directions are **fire-and-forget, with no retry**: a failed dispatch is logged (naming the node and the gRPC status) and dropped in the dispatch second — the deploy path's 2-minute apply deadline and a node's boot-time replay (`DriverHostActor.Bootstrap()` re-applying an orphan `Applying` row) are the only recovery mechanisms, exactly as they were under `ClusterClient`.

@@ -282,12 +282,14 @@ public sealed class CentralCommunicationActor : ReceiveActor, IWithTimers
    }

    /// <summary>Re-arms a single-shot <see cref="RefreshContacts"/> while the gate is unpublished.</summary>
-    private void ScheduleBootstrapRetry()
+    /// <returns><see langword="false"/> when the retry budget is spent and nothing was scheduled.</returns>
+    private bool ScheduleBootstrapRetry()
    {
        if (_bootstrapRetries >= MaxBootstrapRetries)
-            return;
+            return false;
        _bootstrapRetries++;
        Timers.StartSingleTimer(BootstrapRetryTimerKey, new RefreshContacts(), _bootstrapRetryInterval);
+        return true;
    }

    /// <summary>
@@ -758,12 +760,11 @@ public sealed class CentralCommunicationActor : ReceiveActor, IWithTimers
            // Gated on a factory having been supplied at all: a never-supplied factory is a
            // deliberate wiring/test choice Resolve() already stayed silent about (see its
            // remarks), not a resolve failure, so it earns no Error here either.
-            if (_pskProviderFactory is not null && !_gatePublished)
-                ScheduleBootstrapRetry();
-
-            // Error only once the bounded fast retry is exhausted, so a healed boot race stays quiet.
-            if (_pskProviderFactory is not null && !_pskProviderAbsenceReported
-                && _bootstrapRetries >= MaxBootstrapRetries)
+            // Error only when the scheduler REFUSES (budget spent and this final attempt also
+            // failed), so a healed boot race stays quiet.
+            var budgetSpent = _pskProviderFactory is not null && !_gatePublished
+                && !ScheduleBootstrapRetry();
+            if (budgetSpent && !_pskProviderAbsenceReported)
            {
                _pskProviderAbsenceReported = true;
                _log.Error(
@@ -747,20 +747,32 @@ public class CentralCommunicationActorTests : ControlPlaneActorTestBase
    [Fact]
    public void Bootstrap_retries_stop_once_the_gate_is_published()
    {
+        // The provider throws on PreStart (call 1) and on the first load's resolve (call 2), which
+        // schedules a 1 s retry. A manual refresh then publishes while that retry is still PENDING;
+        // stop-on-publish must cancel it. Without the cancel the timer fires, loads again and
+        // republishes (loads == 3, publications == 2).
        var inner = NewInMemoryDbFactory();
        SeedGrpcCluster(inner, "C1", ("host-a:4053", "host-a", 4056, true, false));
        var factory = new CountingDbFactory(inner);
        var psk = new RecordingPskProvider();
-        Sys.ActorOf(CentralCommunicationActor.Props(
-            factory, GrpcMode(), () => null, () => new RecordingDialer(), () => psk,
-            bootstrapRetryInterval: TimeSpan.FromMilliseconds(100)));
+        var calls = 0;

-        // Positive control: the gate did publish before we assert anything about "afterwards".
-        AwaitCondition(() => psk.KnownClusterPublications.Count >= 1, TimeSpan.FromSeconds(10));
-        Thread.Sleep(1000);
+        IMeshPskProvider? PskFactory() =>
+            Interlocked.Increment(ref calls) <= 2 ? throw new InvalidOperationException("race") : psk;

-        // Ten retry intervals elapsed with a 60 s periodic cadence: no extra loads, no republish.
-        factory.Calls.ShouldBe(1);
+        var actor = Sys.ActorOf(CentralCommunicationActor.Props(
+            factory, GrpcMode(), () => null, () => new RecordingDialer(), PskFactory,
+            bootstrapRetryInterval: TimeSpan.FromSeconds(1)));
+
+        // Positive control: the failed first resolve happened, so a retry is pending.
+        AwaitCondition(() => Volatile.Read(ref calls) >= 2, TimeSpan.FromSeconds(5));
+        Thread.Sleep(100);
+        actor.Tell(new CentralCommunicationActor.RefreshContacts());
+        AwaitCondition(() => psk.KnownClusterPublications.Count >= 1, TimeSpan.FromSeconds(5));
+
+        Thread.Sleep(2500);
+
+        factory.Calls.ShouldBe(2);
        psk.KnownClusterPublications.Count.ShouldBe(1);
    }