From 6112a9f4477baf5931059315f9735e2a279165e4 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 9 Nov 2026 14:54:53 -0500 Subject: [PATCH] fix(secrets): Akka replication actor starts with the host (scadaproj#33) The replication actor was created only when something resolved ISecretStore (store -> replicator -> SecretReplicationActorProvider.ActorRef). A node that expanded its ${secret:} references pre-host and never touched the store at runtime had no actor: no topic subscription, no announces, no pull replies. AddZbSecretsAkkaReplication now registers SecretReplicationActorStarter, which creates the actor on IHostApplicationLifetime.ApplicationStarted (after every hosted service, incl. an Akka.Hosting AkkaHostedService registered later), or in StartAsync when no lifetime is registered. - tests: EagerActorStartTests (generic host in both registration orders with a no-actor-before-start control; bare container) - all fail without the fix - docs: README, CHANGELOG - code-reviews: Secrets-010 Resolved; README regenerated Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV --- ZB.MOM.WW.Secrets/CHANGELOG.md | 8 ++ ZB.MOM.WW.Secrets/README.md | 4 +- .../AkkaSecretsServiceCollectionExtensions.cs | 17 ++- .../SecretReplicationActorStarter.cs | 73 ++++++++++++ .../EagerActorStartTests.cs | 109 ++++++++++++++++++ ...Secrets.Replicator.AkkaDotNet.Tests.csproj | 1 + code-reviews/README.md | 11 +- code-reviews/Secrets/findings.md | 6 +- 8 files changed, 214 insertions(+), 15 deletions(-) create mode 100644 ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet/DependencyInjection/SecretReplicationActorStarter.cs create mode 100644 ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests/DependencyInjection/EagerActorStartTests.cs diff --git a/ZB.MOM.WW.Secrets/CHANGELOG.md b/ZB.MOM.WW.Secrets/CHANGELOG.md index c4996e6..bc46024 100644 --- a/ZB.MOM.WW.Secrets/CHANGELOG.md +++ b/ZB.MOM.WW.Secrets/CHANGELOG.md @@ -35,6 +35,14 @@ Fixes six scadaproj#15 code-review findings (`code-reviews/Secrets/findings.md`) token. Behaviour changes: a referencing key keeps its startup value until restart (edits to it are ignored on reload); every reference is resolved before anything is written, so a fail-closed miss leaves the configuration untouched instead of half expanded. +- **The Akka replication actor starts with the host** (scadaproj#33, Secrets-010). It used to be created + only when something resolved `ISecretStore`, so a node that expanded its references pre-host and never + touched the store at runtime had no actor and silently missed every rotation. `AddZbSecretsAkkaReplication` + now registers a hosted starter that creates it on `IHostApplicationLifetime.ApplicationStarted` (after + every hosted service, including an Akka.Hosting `AkkaHostedService` registered later), or in its own + `StartAsync` when no lifetime is registered. Behaviour change: the application's `ActorSystem` must be + resolvable once the host has started even if no secret is ever touched; a failure there is logged. + OtOpcUa's own `SecretReplicationStarter` becomes redundant (harmless — same actor). ## 0.6.5 — unreleased (NOT published to the feed) diff --git a/ZB.MOM.WW.Secrets/README.md b/ZB.MOM.WW.Secrets/README.md index 5aaa6b6..b8579c1 100644 --- a/ZB.MOM.WW.Secrets/README.md +++ b/ZB.MOM.WW.Secrets/README.md @@ -183,7 +183,9 @@ builder.Services.AddZbSecretsAkkaReplication(builder.Configuration); Config config = AkkaSecretsReplication.SerializationConfig.WithFallback(myAppConfig); ``` -Requires the application to register its `ActorSystem` in DI. The local store is SQLite. This is +Requires the application to register its `ActorSystem` in DI. Since `0.6.6` the replication actor +is created when the host has started, so a node replicates whether or not anything on it ever +resolves a secret. The local store is SQLite. This is the right answer for air-gapped or intermittently-connected sites; it is the wrong answer if every node can always reach a shared database, because you pay real distributed-systems behaviour for it. diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet/DependencyInjection/AkkaSecretsServiceCollectionExtensions.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet/DependencyInjection/AkkaSecretsServiceCollectionExtensions.cs index ab3db92..99f7bb8 100644 --- a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet/DependencyInjection/AkkaSecretsServiceCollectionExtensions.cs +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet/DependencyInjection/AkkaSecretsServiceCollectionExtensions.cs @@ -30,8 +30,9 @@ public static class AkkaSecretsServiceCollectionExtensions /// distributed failure modes at all; prefer it unless partition tolerance is a stated requirement. /// /// - /// Requirements. The application must (1) register its in DI - /// before the first secret resolve, and (2) give every node the same KEK — rows carry + /// Requirements. The application must (1) register its in DI — + /// the replication actor is created once the host has started (after every hosted service, + /// when IHostApplicationLifetime is available), so the system must be resolvable by then — and (2) give every node the same KEK — rows carry /// ciphertext only, so a node with a different master key fails closed on a kek_id /// mismatch. Merging into the actor /// system's configuration is recommended. @@ -73,9 +74,10 @@ public static class AkkaSecretsServiceCollectionExtensions section.Bind(eager); eager.Validate(); - // The actor is created lazily on first use rather than at registration: the ActorSystem is - // typically registered by the application AFTER this call, and a cluster node may also not be - // ready to join at the moment the container is built. + // The actor is created lazily rather than at registration: the ActorSystem is typically + // registered by the application AFTER this call, and a cluster node may also not be ready to + // join at the moment the container is built. SecretReplicationActorStarter (registered below) + // forces it into existence once the host has started (scadaproj#33). // // The invalidator MUST stay deferred (scadaproj#1): it is the resolver, the resolver reads // ISecretStore, and ISecretStore is the ReplicatingSecretStore decorator registered below — @@ -111,6 +113,11 @@ public static class AkkaSecretsServiceCollectionExtensions sp.GetRequiredService(), sp.GetRequiredService>())); + // Create the actor at host start rather than on the first ISecretStore resolve + // (scadaproj#33), so a node that never touches the store at runtime still replicates. + // AddHostedService de-duplicates, so a repeated call registers one starter. + services.AddHostedService(); + return services; } } diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet/DependencyInjection/SecretReplicationActorStarter.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet/DependencyInjection/SecretReplicationActorStarter.cs new file mode 100644 index 0000000..3d19c98 --- /dev/null +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet/DependencyInjection/SecretReplicationActorStarter.cs @@ -0,0 +1,73 @@ +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging; + +namespace ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.DependencyInjection; + +/// +/// Creates this node's replication actor at host start, so participation in replication does not +/// depend on something happening to resolve ISecretStore (scadaproj#33). +/// +/// +/// +/// The actor is created by , which until 0.6.6 +/// was read only through the ISecretStore → ISecretReplicator chain. A node that +/// expanded its ${secret:} references pre-host and never touched the store at runtime had no +/// actor: no topic subscription, no announces, no pull replies — it silently missed every rotation +/// and served the stale credential on its next restart. +/// +/// +/// When the host exposes , creation waits for +/// — after EVERY hosted service has started, +/// including an Akka.Hosting AkkaHostedService that may be registered after this package — so +/// the starter never races the application's own actor-system startup, whatever the registration +/// order. A failure there is logged, because nothing can propagate it; the actor provider caches the +/// failure, so the first store resolve rethrows it as before. Without a lifetime (a bare container +/// whose hosted services are started by hand) the actor is created inside +/// and a failure propagates. +/// +/// +/// Resolving does not re-enter the scadaproj#1 cycle: +/// its cache invalidator is still the deferred one, resolved only at the first eviction. +/// +/// +internal sealed class SecretReplicationActorStarter( + IServiceProvider services, + ILogger logger) : IHostedService +{ + /// + public Task StartAsync(CancellationToken cancellationToken) + { + IHostApplicationLifetime? lifetime = services.GetService(); + if (lifetime is null) + { + CreateActor(); + return Task.CompletedTask; + } + + lifetime.ApplicationStarted.Register(() => + { + try + { + CreateActor(); + } + catch (Exception ex) + { + logger.LogError( + ex, + "The secret replication actor could not be created at startup; this node will not " + + "receive or announce secret changes until the cause is fixed."); + } + }); + + return Task.CompletedTask; + } + + /// + public Task StopAsync(CancellationToken cancellationToken) => Task.CompletedTask; + + // Reading ActorRef is the side effect: the provider's Lazy spawns the actor exactly once, shared + // with every later ISecretReplicator resolve. + private void CreateActor() => + _ = services.GetRequiredService().ActorRef; +} diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests/DependencyInjection/EagerActorStartTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests/DependencyInjection/EagerActorStartTests.cs new file mode 100644 index 0000000..2bc1406 --- /dev/null +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests/DependencyInjection/EagerActorStartTests.cs @@ -0,0 +1,109 @@ +using Akka.Actor; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using ZB.MOM.WW.Secrets.Abstractions; +using ZB.MOM.WW.Secrets.DependencyInjection; +using ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.DependencyInjection; + +namespace ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests.DependencyInjection; + +/// +/// scadaproj#33 (Secrets-010): the replication actor must exist once the host has started, without +/// anything resolving . Before the fix the actor was created only through +/// the store → replicator chain, so a node that expanded its references pre-host and never touched +/// the store at runtime had no actor and silently missed every rotation. +/// +/// +/// Nothing in these tests may resolve , or +/// before the assertion — that would create the actor the old way and +/// make the test vacuous. could not catch the defect for +/// exactly that reason. +/// +public sealed class EagerActorStartTests(SingleNodeClusterFixture cluster) + : IClassFixture +{ + public enum Order + { + CoreFirst, + ExtensionFirst, + } + + private static Dictionary Settings(string actorName) => new() + { + ["Secrets:SqlitePath"] = Path.Combine(Path.GetTempPath(), $"zb-eager-{Guid.NewGuid():N}.db"), + ["Secrets:RunMigrationsOnStartup"] = "true", + ["Secrets:Replication:AnnounceInterval"] = "00:00:30", + ["Secrets:Replication:ActorName"] = actorName, + }; + + private async Task ActorExistsAsync(string actorName) + { + try + { + await cluster.System.ActorSelection($"/user/{actorName}").ResolveOne(TimeSpan.FromSeconds(3)); + return true; + } + catch (ActorNotFoundException) + { + return false; + } + } + + [Theory] + [InlineData(Order.CoreFirst)] + [InlineData(Order.ExtensionFirst)] + public async Task A_started_generic_host_creates_the_actor_without_resolving_the_store(Order order) + { + string actorName = $"zb-eager-host-{order}"; + HostApplicationBuilder builder = Host.CreateApplicationBuilder(new HostApplicationBuilderSettings + { + Args = [], + DisableDefaults = true, + }); + builder.Configuration.AddInMemoryCollection(Settings(actorName)); + builder.Services.AddSingleton(cluster.System); + if (order == Order.CoreFirst) + { + builder.Services.AddZbSecrets(builder.Configuration, "Secrets"); + builder.Services.AddZbSecretsAkkaReplication(builder.Configuration); + } + else + { + builder.Services.AddZbSecretsAkkaReplication(builder.Configuration); + builder.Services.AddZbSecrets(builder.Configuration, "Secrets"); + } + + using IHost host = builder.Build(); + Assert.False(await ActorExistsAsync(actorName), "positive control: no actor before the host starts"); + + await host.StartAsync(); + try + { + Assert.True(await ActorExistsAsync(actorName), "the replication actor was not created at startup"); + } + finally + { + await host.StopAsync(); + } + } + + [Fact] + public async Task Without_a_host_lifetime_the_starter_creates_the_actor_in_StartAsync() + { + string actorName = "zb-eager-bare"; + IConfigurationRoot config = new ConfigurationBuilder().AddInMemoryCollection(Settings(actorName)).Build(); + var services = new ServiceCollection(); + services.AddLogging(); + services.AddSingleton(cluster.System); + services.AddZbSecretsAkkaReplication(config); + await using ServiceProvider provider = services.BuildServiceProvider(); + + foreach (IHostedService hosted in provider.GetServices()) + { + await hosted.StartAsync(CancellationToken.None); + } + + Assert.True(await ActorExistsAsync(actorName), "the replication actor was not created at startup"); + } +} diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests.csproj b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests.csproj index d7118b4..78376b2 100644 --- a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests.csproj +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests.csproj @@ -13,6 +13,7 @@ + diff --git a/code-reviews/README.md b/code-reviews/README.md index c669915..14fc7a1 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -40,7 +40,7 @@ code-reviews/ ## Summary -10 of 10 libraries reviewed. 27 pending findings across all libraries. +10 of 10 libraries reviewed. 26 pending findings across all libraries. Coverage: Secrets, LocalDb, GalaxyRepository and Overview first reviewed, and Auth, Health, Theme, Audit, Configuration and Telemetry re-reviewed, on 2026-11-09 (#15). Each finding also @@ -50,9 +50,9 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low). |----------|---------------| | Critical | 0 | | High | 0 | -| Medium | 8 | +| Medium | 7 | | Low | 19 | -| **Total** | **27** | +| **Total** | **26** | ## Library Status @@ -65,7 +65,7 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low). | [Health](Health/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 7 | | [LocalDb](LocalDb/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/4/3 | 7 | 7 | | [Overview](Overview/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 5 | -| [Secrets](Secrets/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/3/13 | 16 | 19 | +| [Secrets](Secrets/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/2/13 | 15 | 19 | | [Telemetry](Telemetry/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 11 | | [Theme](Theme/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 8 | @@ -84,7 +84,7 @@ _None open._ _None open._ -### Medium (8) +### Medium (7) | ID | Library | Title | |----|---------|-------| @@ -93,7 +93,6 @@ _None open._ | LocalDb-002 | [LocalDb](LocalDb/findings.md) | Snapshot streams tables by name and rows by `pk_json` text, so FK children land before parents and are dead-lettered for good | | LocalDb-003 | [LocalDb](LocalDb/findings.md) | `INSERT OR REPLACE` apply silently deletes a different row on a secondary `UNIQUE` conflict, so the pair diverges | | LocalDb-004 | [LocalDb](LocalDb/findings.md) | `FailClosedOnDrift` is bypassed on the snapshot path, so a future-stamped snapshot row moves the local HLC forward permanently | -| Secrets-010 | [Secrets](Secrets/findings.md) | Akka replication actor is created only when `ISecretStore` is first resolved; a node that never resolves it never replicates | | Secrets-013 | [Secrets](Secrets/findings.md) | UI add/rotate/delete: if the audit write throws after the store write succeeded, the change is reported and audited as a failure and the resolver cache is not evicted | | Secrets-017 | [Secrets](Secrets/findings.md) | `secret set`/`rotate` take the plaintext on argv; the host builder re-parses argv as configuration and can crash with the value in the error | diff --git a/code-reviews/Secrets/findings.md b/code-reviews/Secrets/findings.md index f057613..03a9e1f 100644 --- a/code-reviews/Secrets/findings.md +++ b/code-reviews/Secrets/findings.md @@ -10,7 +10,7 @@ | Last reviewed | 2026-11-09 | | Reviewer | Claude (scadaproj#15) | | Commit reviewed | `91fb2b4` | -| Open findings | 16 | +| Open findings | 15 | ## Summary @@ -328,7 +328,7 @@ _Unresolved._ | Severity | Medium | | Priority | P2 | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet/DependencyInjection/AkkaSecretsServiceCollectionExtensions.cs:79` | **Description** @@ -360,7 +360,7 @@ Register a small `IHostedService` in `AddZbSecretsAkkaReplication` that reads `S **Resolution** -_Unresolved._ +Resolved 2026-11-09 in the commit tagged `(scadaproj#33)` (Secrets `0.6.6`, unpublished). `AddZbSecretsAkkaReplication` registers an internal `SecretReplicationActorStarter` hosted service that reads `SecretReplicationActorProvider.ActorRef`. When `IHostApplicationLifetime` is present it does so on `ApplicationStarted` — after every hosted service, so it cannot race an Akka.Hosting `AkkaHostedService` registered after the package — and logs a failure (the provider's `Lazy` caches it, so the first store resolve still rethrows); without a lifetime it creates the actor inside `StartAsync` and lets a failure propagate. Resolving the provider does not touch the deferred invalidator, so the scadaproj#1 cycle is not re-entered (the full Akka suite, including `HostedProcessResolutionTests`, passes). Regression tests `EagerActorStartTests`: a started generic host in both registration orders (with a positive control that no actor exists before start) and a bare container with hosted services started by hand; all three fail without the starter. Nothing in them resolves `ISecretStore`. ### Secrets-011 — `HandleAnnounceTick` publishes from a thread-pool continuation: announces carry no sender under an async store, and faults are silently dropped