From 2659a690a62645db7c903026698de659cd48c668 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 8 Oct 2026 23:39:06 -0400 Subject: [PATCH] fix(#654): review fixes - non-vacuous stop-on-publish test, Error only when retry budget refuses Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- docs/Configuration.md | 2 +- .../CentralCommunicationActor.cs | 17 +++++------ .../CentralCommunicationActorTests.cs | 28 +++++++++++++------ 3 files changed, 30 insertions(+), 17 deletions(-) diff --git a/docs/Configuration.md b/docs/Configuration.md index 7bed57f9..6fcd5456 100644 --- a/docs/Configuration.md +++ b/docs/Configuration.md @@ -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`. 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 d301d9da..0c18eaa5 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Communication/CentralCommunicationActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Communication/CentralCommunicationActor.cs @@ -282,12 +282,14 @@ public sealed class CentralCommunicationActor : ReceiveActor, IWithTimers } /// Re-arms a single-shot while the gate is unpublished. - private void ScheduleBootstrapRetry() + /// when the retry budget is spent and nothing was scheduled. + private bool ScheduleBootstrapRetry() { if (_bootstrapRetries >= MaxBootstrapRetries) - return; + return false; _bootstrapRetries++; Timers.StartSingleTimer(BootstrapRetryTimerKey, new RefreshContacts(), _bootstrapRetryInterval); + return true; } /// @@ -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( 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 c18a3427..d4e8a1fb 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 @@ -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); }