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