From 2254ae3deaccdda630591edacfeb4e670e4c6901 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 18 Jul 2026 15:03:35 -0400 Subject: [PATCH] fix(secrets): consume Secrets 0.2.2 - clustered-secrets DI deadlock fixed upstream Bumps the four ZB.MOM.WW.Secrets pins 0.2.1 -> 0.2.2 (closes the OtOpcUa side of scadaproj#1, tracked here as #482). 0.2.1's Akka replicator deadlocked any hosted process at startup when Secrets:Replication:Enabled was true: the package's DI graph closed a circular singleton dependency through factory lambdas (store decorator -> replicator -> actor provider -> cache invalidator -> resolver -> store), which MS.DI's StackGuard turns into a silent cross-thread call-site-lock deadlock. 0.2.2 defers the invalidator edge to first eviction. The flag stays default-false; enabling remains a per-environment decision. The_startup_hook_actually_creates_the_replication_actor is now a real test: SecretReplicationStarter's docs had promised it since the adoption, and the upstream fix finally makes a provider-based resolve runnable - container built exactly as the host does, hook started under a watchdog, replication actor proven to exist by ActorSelection on a self-joined single-node cluster (no TestKit needed, which matters because Akka.TestKit.Xunit2 is xunit-v2-only and this project is on xunit.v3). Also corrects the stale rationale that blamed the old hang on DistributedPubSub needing a joined cluster - the actor constructor was never reached; it was the DI cycle. Verified: SecretsReplicationRegistrationTests 8/8 on the 0.2.2 feed packages; full slnx build 0 errors; the 2-node Akka live convergence gate re-run against the published 0.2.2 packages passes 6/6 (write->peer, tombstone propagation without resurrection, delete visibility through the resolver cache, reverse direction, wrong-KEK fail-closed). Claude-Session: https://claude.ai/code/session_01BL2Vu1ESDQ9SCN4gVKkdts --- Directory.Packages.props | 8 +- .../Configuration/SecretReplicationStarter.cs | 18 ++-- .../SecretsReplicationRegistrationTests.cs | 84 +++++++++++++++---- 3 files changed, 80 insertions(+), 30 deletions(-) diff --git a/Directory.Packages.props b/Directory.Packages.props index f77a355f..f62cbbdb 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -132,10 +132,10 @@ - - - - + + + + diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/SecretReplicationStarter.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/SecretReplicationStarter.cs index b6d1a2cd..2d3bc042 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/SecretReplicationStarter.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/SecretReplicationStarter.cs @@ -24,15 +24,15 @@ namespace ZB.MOM.WW.OtOpcUa.Host.Configuration; /// unconditional. /// /// -/// Known ineffective against ZB.MOM.WW.Secrets.Replicator.AkkaDotNet 0.2.0. That -/// version never binds its own ISecretReplicator: -/// AddZbSecretsAkkaReplication calls AddZbSecrets first, which -/// TryAdds NoOpSecretReplicator, making the package's own subsequent -/// TryAddSingleton<ISecretReplicator> a no-op. Resolving the store therefore -/// builds a ReplicatingSecretStore around a no-op replicator and spawns no actor. -/// This hook is correct and stays in place for when the library is fixed, but replication -/// must be treated as non-functional until then — see the skipped test -/// SecretsReplicationRegistrationTests.The_startup_hook_actually_creates_the_replication_actor. +/// History — this hook has met two library defects, both fixed. Against 0.2.0 it was +/// inert: the package never bound its own ISecretReplicator (TryAdd registration +/// order), so this resolve built a ReplicatingSecretStore around a no-op sink and +/// spawned no actor. Against 0.2.1 it was worse: the resolve deadlocked the host at +/// startup — the package's DI graph had a circular singleton dependency, invisible to +/// the container through factory lambdas (scadaproj#1). Fixed in 0.2.2 by deferring the +/// cycle-closing invalidator edge; +/// SecretsReplicationRegistrationTests.The_startup_hook_actually_creates_the_replication_actor +/// exercises this hook against a built provider on a real single-node cluster. /// /// /// Root provider used to resolve the (decorated) secret store exactly once. diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/SecretsReplicationRegistrationTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/SecretsReplicationRegistrationTests.cs index cbaee73c..fcbe2537 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/SecretsReplicationRegistrationTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/SecretsReplicationRegistrationTests.cs @@ -103,23 +103,13 @@ public sealed class SecretsReplicationRegistrationTests [Fact] public void Replication_enabled_decorates_the_store_so_writes_publish() { - // Asserted on the ServiceCollection rather than a built provider, deliberately. Resolving - // ISecretStore with replication on constructs ReplicatingSecretStore, which resolves - // ISecretReplicator, which eagerly spawns the replication actor, whose PreStart calls - // DistributedPubSub.Get(...) — unavailable until the node has joined a cluster. Against a - // plain ActorSystem that resolve hangs instead of failing. - // - // Akka.TestKit is the family convention for this (ScadaBridge's suites use it throughout) - // and would be the right tool, but it is NOT available here: Akka.TestKit.Xunit2 is - // xunit-v2-only, and this project is one of the 44 on xunit.v3. Adding it yields CS0433 - // type conflicts. Directory.Packages.props documents the same constraint — AdminUI.Tests, - // ControlPlane.Tests and Runtime.Tests are deliberately held on xunit v2 precisely because - // no xunit.v3 TestKit ships as of Akka 1.5.62. Host wiring tests belong here, not in one of - // those three, so the descriptor is the right seam until Akka ships a v3 TestKit. - // - // Nothing is lost: the registration order IS the defect, and the library's own - // TwoNodeClusterReplicationTests already cover actor creation and convergence against a - // genuine 2-node cluster. + // Asserted on the ServiceCollection because the registration order IS the defect class this + // file guards (which descriptor won a TryAdd race), and the descriptor is where that is + // visible. An earlier revision of this comment ALSO claimed a provider-based resolve was + // impossible ("hangs against a plain ActorSystem — DistributedPubSub needs a joined + // cluster"). That hang was real but misattributed: it was a circular singleton dependency + // in the 0.2.1 package's own DI wiring (scadaproj#1), fixed in 0.2.2. The provider-based + // resolve is covered by The_startup_hook_actually_creates_the_replication_actor below. // // AddSingleton (not TryAdd) appends the decorator, and the LAST registration for a service // type is what the container resolves. @@ -130,6 +120,66 @@ public sealed class SecretsReplicationRegistrationTests "with replication enabled the local store must be decorated so writes publish to peers"); } + [Fact] + public async Task The_startup_hook_actually_creates_the_replication_actor() + { + // The test SecretReplicationStarter's docs have promised since the 0.2.x adoption, runnable + // now that the upstream deadlock is fixed: build the container the way the host does, start + // the hook, and prove the replication actor exists. On 0.2.0 this fails because the actor + // is never spawned (inert replicator); on 0.2.1 it deadlocks in the resolve (scadaproj#1). + // + // Akka.TestKit.Xunit2 is xunit-v2-only and this project is on xunit.v3 (CS0433), so this + // uses a plain self-joined single-node cluster — which is also all the actor needs: its + // constructor gets the DistributedPubSub mediator, no peers required. + ActorSystem system = ActorSystem.Create( + "otopcua-secrets-gate", + Akka.Configuration.ConfigurationFactory.ParseString(""" + akka { + loglevel = WARNING + actor.provider = cluster + remote.dot-netty.tcp { + hostname = "127.0.0.1" + public-hostname = "127.0.0.1" + port = 0 + } + } + """)); + + try + { + var cluster = Akka.Cluster.Cluster.Get(system); + cluster.Join(cluster.SelfAddress); + + using ServiceProvider sp = BuildProvider(replicationEnabled: true, system); + + // Startup order mirrors the host: hosted services run, nothing else has resolved the + // store yet. The hook's resolve is the moment 0.2.1 hung forever, so it runs under a + // watchdog — a regression should fail the test, not the whole run. + var starter = sp.GetServices().OfType().Single(); + + Task start = starter.StartAsync(TestContext.Current.CancellationToken); + Task first = await Task.WhenAny( + start, Task.Delay(TimeSpan.FromSeconds(20), TestContext.Current.CancellationToken)); + first.ShouldBe(start, + "resolving ISecretStore from the startup hook did not complete — the scadaproj#1 " + + "DI deadlock has regressed"); + await start; + + // The store must be the replicating decorator, and the node's replication actor must + // genuinely exist under the configured name — not merely be registered. + sp.GetRequiredService().ShouldBeOfType(); + + IActorRef actor = await system + .ActorSelection("/user/zb-secret-replication") + .ResolveOne(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken); + actor.Path.Name.ShouldBe("zb-secret-replication"); + } + finally + { + await system.Terminate(); + } + } + [Fact] public void Replication_enabled_still_registers_the_undecorated_concrete_store() {