From 015c556ced3066e4bd2a639806a84c720a4223d2 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 9 Nov 2026 14:47:01 -0500 Subject: [PATCH] fix(secrets): SqlServer/Akka registration is order-independent; bump 0.6.6 (scadaproj#30) AddZbSecretsSqlServerStore / AddZbSecretsSqlServerReplication (and the Akka extension, same defect class) TryAdd-ed their ISecretStore, ISecretsStoreMigrator and ISecretReplicator ahead of their internal AddZbSecrets call. An application that had already called AddZbSecrets therefore silently kept the SQLite store (shared-store mode) or NoOpSecretReplicator (hub/Akka modes). They now call AddZbSecrets first and RemoveAll + AddSingleton their own registrations; a later AddZbSecrets only TryAdds, so either order yields the same graph with exactly one ISecretStore registration. - tests: RegistrationOrderTests (SqlServer + Akka) build and resolve the real container in both orders; core-first fails without the fix - Directory.Build.props 0.6.5 -> 0.6.6 (NOT packed or published) - code-reviews: Secrets-001 Resolved; README regenerated Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV --- ZB.MOM.WW.Secrets/CHANGELOG.md | 14 +++ ZB.MOM.WW.Secrets/CLAUDE.md | 2 + ZB.MOM.WW.Secrets/Directory.Build.props | 2 +- ZB.MOM.WW.Secrets/README.md | 4 + .../AkkaSecretsServiceCollectionExtensions.cs | 21 +++-- ...erverSecretsServiceCollectionExtensions.cs | 32 ++++--- .../SecretsServiceCollectionExtensions.cs | 6 +- .../RegistrationOrderTests.cs | 61 +++++++++++++ .../SingleNodeClusterFixture.cs | 49 +++++++++++ .../RegistrationOrderTests.cs | 86 +++++++++++++++++++ code-reviews/README.md | 11 ++- code-reviews/Secrets/findings.md | 6 +- 12 files changed, 263 insertions(+), 31 deletions(-) create mode 100644 ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests/DependencyInjection/RegistrationOrderTests.cs create mode 100644 ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests/DependencyInjection/SingleNodeClusterFixture.cs create mode 100644 ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.SqlServer.Tests/DependencyInjection/RegistrationOrderTests.cs diff --git a/ZB.MOM.WW.Secrets/CHANGELOG.md b/ZB.MOM.WW.Secrets/CHANGELOG.md index 7b48719..54c36e8 100644 --- a/ZB.MOM.WW.Secrets/CHANGELOG.md +++ b/ZB.MOM.WW.Secrets/CHANGELOG.md @@ -5,6 +5,20 @@ follows [Keep a Changelog](https://keepachangelog.com/); versions are shared acr package in the family (see `Directory.Build.props`) even when a given release only touches one of them. +## 0.6.6 — unreleased (NOT published to the feed) + +Fixes six scadaproj#15 code-review findings (`code-reviews/Secrets/findings.md`). + +### Fixed + +- **SQL-Server and Akka registration no longer depends on call order** (scadaproj#30, Secrets-001). + `AddZbSecretsSqlServerStore`, `AddZbSecretsSqlServerReplication` and `AddZbSecretsAkkaReplication` + used to `TryAdd` their `ISecretStore` / `ISecretsStoreMigrator` / `ISecretReplicator` ahead of their + internal `AddZbSecrets` call, so an application that had already called `AddZbSecrets` silently kept + the SQLite store (shared-store mode) or the `NoOpSecretReplicator` (hub and Akka modes). They now + replace those registrations after the core call; a later `AddZbSecrets` only `TryAdd`s, so either + order yields the same graph, and exactly one `ISecretStore` is registered. + ## 0.6.5 — unreleased (NOT published to the feed) ### Changed diff --git a/ZB.MOM.WW.Secrets/CLAUDE.md b/ZB.MOM.WW.Secrets/CLAUDE.md index e3905cb..72bee1b 100644 --- a/ZB.MOM.WW.Secrets/CLAUDE.md +++ b/ZB.MOM.WW.Secrets/CLAUDE.md @@ -4,6 +4,8 @@ Shared encrypted secret store + `${secret:}` resolution for the **ZB.MOM.WW SCAD **Backlog / open work:** lives in Gitea issues on `dohertj2/scadaproj` (`tea issues list --repo dohertj2/scadaproj`) — currently #7 (ignitionoee router pins), #8 (Replicator.Grpc / `0.6.3` fate), #18 (migrator retry jitter). The history below is a record, not a status tracker; file new follow-ups as issues. +**`0.6.6` — built, NOT published (2026-11-09, scadaproj#30–#35):** six scadaproj#15 review fixes — see `CHANGELOG.md` and `../code-reviews/Secrets/findings.md` (Secrets-001/002/003/010/013/017). + **`0.6.5` — built, NOT published (2026-11-09, scadaproj#26/#27):** `Microsoft.Data.Sqlite` 10.0.7 -> 10.0.13 (efcore#39009 pool fault) with `SQLitePCLRaw.lib.e_sqlite3` pin 2.1.12 -> 2.1.13; Windows test fixes. See `CHANGELOG.md`. **`0.6.4` — built, NOT published (2026-11-09, scadaproj#25):** `Directory.Build.props` says `0.6.4` but the feed's newest is still `0.6.3`. Concurrent SQLite migration of one store is now gated per normalised path, runs on an unpooled `BEGIN IMMEDIATE` connection and retries `SQLITE_BUSY`/`SQLITE_LOCKED` (see `CHANGELOG.md`). Tests: `tests/ZB.MOM.WW.Secrets.Tests/Sqlite/SqliteSecretsStoreConcurrentInitTests.cs` + the gate/retry cases in `SqliteSecretsStoreMigratorTests.cs`; also run on windev. diff --git a/ZB.MOM.WW.Secrets/Directory.Build.props b/ZB.MOM.WW.Secrets/Directory.Build.props index ddd1c05..420e655 100644 --- a/ZB.MOM.WW.Secrets/Directory.Build.props +++ b/ZB.MOM.WW.Secrets/Directory.Build.props @@ -5,7 +5,7 @@ enable enable latest - 0.6.5 + 0.6.6 true README.md diff --git a/ZB.MOM.WW.Secrets/README.md b/ZB.MOM.WW.Secrets/README.md index b58b8d8..cc36558 100644 --- a/ZB.MOM.WW.Secrets/README.md +++ b/ZB.MOM.WW.Secrets/README.md @@ -180,6 +180,10 @@ Requires the application to register its `ActorSystem` in DI. The local store 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. +Since `0.6.6` all three extensions are **order-independent** with respect to `AddZbSecrets`: call +it before or after them (or not at all — each calls it internally) and the same store, migrator and +replicator are bound. + ### What replication costs you (2 and 3) - **Eventual consistency.** Rows converge within the sweep interval, not instantly. 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 e3aa26f..c0aa260 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 @@ -82,20 +82,23 @@ public static class AkkaSecretsServiceCollectionExtensions new DeferredSecretCacheInvalidator(sp.GetService), sp.GetRequiredService>().Value)); - // MUST be registered BEFORE AddZbSecrets. That call does - // TryAddSingleton(), so if it ran first it would - // win and the TryAdd below would be silently discarded — leaving replication bound to the - // no-op sink, publishing nothing, with no actor ever created and no error anywhere. - // Shipped that way in 0.2.0; fixed in 0.2.1. The SqlServer package always had this order. - services.TryAddSingleton(sp => - new AkkaSecretReplicator(sp.GetRequiredService().ActorRef)); - // The local store + migrator come from AddZbSecrets (SQLite by default). services.AddZbSecrets(config, secretsSectionPath); + // REPLACED after the core call rather than TryAdd-ed before it (scadaproj#30). AddZbSecrets + // does TryAddSingleton(); a TryAdd here only won when + // this package ran first, so an application that had already called AddZbSecrets got + // replication bound to the no-op sink — publishing nothing, no actor ever created, no error + // (the 0.2.0 shape, and still reachable from the application's side until 0.6.6). + services.RemoveAll(); + services.AddSingleton(sp => + new AkkaSecretReplicator(sp.GetRequiredService().ActorRef)); + // Decorate the local store so every write publishes. Resolved from the concrete SQLite store, // not ISecretStore — asking the container for ISecretStore here would resolve this very - // decorator and recurse forever. + // decorator and recurse forever. The core's ISecretStore forward is removed first so the + // decorator is the only registration in either call order. + services.RemoveAll(); services.AddSingleton(sp => new ReplicatingSecretStore( sp.GetRequiredService(), sp.GetRequiredService(), diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.SqlServer/DependencyInjection/SqlServerSecretsServiceCollectionExtensions.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.SqlServer/DependencyInjection/SqlServerSecretsServiceCollectionExtensions.cs index 030a00f..4d01243 100644 --- a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.SqlServer/DependencyInjection/SqlServerSecretsServiceCollectionExtensions.cs +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.SqlServer/DependencyInjection/SqlServerSecretsServiceCollectionExtensions.cs @@ -48,14 +48,19 @@ public static class SqlServerSecretsServiceCollectionExtensions { RegisterSqlServerCore(services, config, sqlServerSectionPath); - // Registered BEFORE AddZbSecrets so its TryAdd calls find these already present and leave - // them alone — that is the whole mechanism by which the SQL-Server store displaces SQLite. - services.TryAddSingleton(sp => sp.GetRequiredService()); - services.TryAddSingleton(sp => - sp.GetRequiredService()); - services.AddZbSecrets(config, secretsSectionPath); + // REPLACE, not TryAdd (scadaproj#30): the application may already have called AddZbSecrets, + // whose SQLite ISecretStore / ISecretsStoreMigrator a TryAdd here would silently leave in + // place — each node of a "shared-store" pair would then write its own local file. Replacing + // after the core call makes the outcome independent of call order: a later AddZbSecrets only + // TryAdds, so it cannot displace these either. + services.RemoveAll(); + services.AddSingleton(sp => sp.GetRequiredService()); + services.RemoveAll(); + services.AddSingleton(sp => + sp.GetRequiredService()); + return services; } @@ -87,15 +92,22 @@ public static class SqlServerSecretsServiceCollectionExtensions services.TryAddSingleton(sp => sp.GetRequiredService()); - services.TryAddSingleton(sp => - new SqlServerSecretReplicator(sp.GetRequiredService())); - // The local store + its migrator come from AddZbSecrets (SQLite by default). services.AddZbSecrets(config, secretsSectionPath); + // REPLACE, not TryAdd (scadaproj#30): if the application called AddZbSecrets first, its + // NoOpSecretReplicator is already registered and a TryAdd would silently lose to it — live + // publish disabled, propagation left to the sweep interval, no error anywhere. + services.RemoveAll(); + services.AddSingleton(sp => + new SqlServerSecretReplicator(sp.GetRequiredService())); + // Decorate the local store so every write publishes. Resolved from the concrete SQLite store // rather than ISecretStore, because ISecretStore is what we are replacing — asking the - // container for it here would resolve this very decorator and recurse forever. + // container for it here would resolve this very decorator and recurse forever. Any earlier + // ISecretStore registration (the core's SQLite forward) is removed so the decorator is the + // only one, whichever order the calls came in. + services.RemoveAll(); services.AddSingleton(sp => new ReplicatingSecretStore( sp.GetRequiredService(), sp.GetRequiredService(), diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/DependencyInjection/SecretsServiceCollectionExtensions.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/DependencyInjection/SecretsServiceCollectionExtensions.cs index 11363da..3451622 100644 --- a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/DependencyInjection/SecretsServiceCollectionExtensions.cs +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/DependencyInjection/SecretsServiceCollectionExtensions.cs @@ -138,8 +138,10 @@ public static class SecretsServiceCollectionExtensions // Migrator: singleton, constructed from the already-registered connection factory. Needed // before any store operations so the schema exists. Registered BOTH concretely (for callers // that want the SQLite migrator specifically) and behind the ISecretsStoreMigrator seam that - // the hosted service and CLI consume — a replication package that swaps the store registers - // its own ISecretsStoreMigrator BEFORE calling AddZbSecrets, and TryAdd leaves it in place. + // the hosted service and CLI consume. A package that swaps the store (SQL-Server shared-store + // mode) REPLACES these after its own AddZbSecrets call, and the TryAdds here leave such a + // replacement in place when the application calls AddZbSecrets again afterwards — so the + // outcome does not depend on call order (scadaproj#30). services.TryAddSingleton(sp => new SqliteSecretsStoreMigrator(sp.GetRequiredService())); diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests/DependencyInjection/RegistrationOrderTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests/DependencyInjection/RegistrationOrderTests.cs new file mode 100644 index 0000000..e5caca3 --- /dev/null +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests/DependencyInjection/RegistrationOrderTests.cs @@ -0,0 +1,61 @@ +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using ZB.MOM.WW.Secrets.Abstractions; +using ZB.MOM.WW.Secrets.DependencyInjection; +using ZB.MOM.WW.Secrets.Replication; +using ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.DependencyInjection; + +namespace ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests.DependencyInjection; + +/// +/// scadaproj#30 (Secrets-001), Akka half: the package must bind its own replicator and decorator +/// whether the application calls AddZbSecrets before or after it. Until 0.6.6 the package +/// TryAdd-ed its replicator ahead of its internal AddZbSecrets, which fixed the 0.2.0 order +/// bug only for the package's own call — an application that called AddZbSecrets FIRST still +/// got NoOpSecretReplicator. Builds the real container against a joined cluster and resolves. +/// +public sealed class RegistrationOrderTests(SingleNodeClusterFixture cluster) + : IClassFixture +{ + public enum Order + { + CoreFirst, + ExtensionFirst, + } + + [Theory] + [InlineData(Order.CoreFirst)] + [InlineData(Order.ExtensionFirst)] + public void Replication_is_bound_in_either_order(Order order) + { + string dbPath = Path.Combine(Path.GetTempPath(), $"zb-akka-order-{Guid.NewGuid():N}.db"); + IConfigurationRoot config = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["Secrets:SqlitePath"] = dbPath, + ["Secrets:RunMigrationsOnStartup"] = "false", + ["Secrets:Replication:AnnounceInterval"] = "00:00:30", + ["Secrets:Replication:ActorName"] = $"zb-secret-replication-{order}", + }) + .Build(); + + var services = new ServiceCollection(); + services.AddLogging(); + services.AddSingleton(cluster.System); + if (order == Order.CoreFirst) + { + services.AddZbSecrets(config, "Secrets"); + services.AddZbSecretsAkkaReplication(config, "Secrets", "Secrets:Replication"); + } + else + { + services.AddZbSecretsAkkaReplication(config, "Secrets", "Secrets:Replication"); + services.AddZbSecrets(config, "Secrets"); + } + + using ServiceProvider provider = services.BuildServiceProvider(); + + Assert.IsType(provider.GetRequiredService()); + Assert.IsType(Assert.Single(provider.GetServices())); + } +} diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests/DependencyInjection/SingleNodeClusterFixture.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests/DependencyInjection/SingleNodeClusterFixture.cs new file mode 100644 index 0000000..6d6918c --- /dev/null +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests/DependencyInjection/SingleNodeClusterFixture.cs @@ -0,0 +1,49 @@ +using Akka.Actor; +using Akka.Cluster; +using Akka.Configuration; + +namespace ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests.DependencyInjection; + +/// +/// One self-joined single-node cluster shared by a test class — the miniature app node the +/// replication actor's DistributedPubSub mediator needs. Tests that spawn the actor must give each +/// container its own Secrets:Replication:ActorName, since actor names are unique per system. +/// +public sealed class SingleNodeClusterFixture : IAsyncLifetime +{ + /// The joined, Up actor system. + public ActorSystem System { get; private set; } = null!; + + /// + public async Task InitializeAsync() + { + System = ActorSystem.Create("zb-secrets-order", 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 + } + } + """).WithFallback(AkkaSecretsReplication.SerializationConfig)); + + Cluster cluster = Cluster.Get(System); + cluster.Join(cluster.SelfAddress); + + DateTime deadline = DateTime.UtcNow + TimeSpan.FromSeconds(20); + while (cluster.State.Members.All(m => m.Status != MemberStatus.Up)) + { + if (DateTime.UtcNow > deadline) + { + throw new TimeoutException("Single-node cluster never reached Up."); + } + + await Task.Delay(100); + } + } + + /// + public async Task DisposeAsync() => await System.Terminate(); +} diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.SqlServer.Tests/DependencyInjection/RegistrationOrderTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.SqlServer.Tests/DependencyInjection/RegistrationOrderTests.cs new file mode 100644 index 0000000..2d372d6 --- /dev/null +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.SqlServer.Tests/DependencyInjection/RegistrationOrderTests.cs @@ -0,0 +1,86 @@ +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using ZB.MOM.WW.Secrets.Abstractions; +using ZB.MOM.WW.Secrets.DependencyInjection; +using ZB.MOM.WW.Secrets.Replication; +using ZB.MOM.WW.Secrets.Replicator.SqlServer.DependencyInjection; +using ZB.MOM.WW.Secrets.Sqlite; + +namespace ZB.MOM.WW.Secrets.Replicator.SqlServer.Tests.DependencyInjection; + +/// +/// scadaproj#30 (Secrets-001): the SQL-Server extensions must produce the same graph whether the +/// application calls AddZbSecrets before or after them. +/// +/// +/// Before the fix both modes put their registrations in with TryAdd ahead of their own +/// internal AddZbSecrets call, so an application that had ALREADY called AddZbSecrets +/// — the order the README teaches, and the only way to pass contentRootPath — silently kept +/// the SQLite store (shared-store mode) or the no-op replicator (hub mode). Every test here builds the +/// real container and resolves, in both orders. +/// +public sealed class RegistrationOrderTests +{ + public enum Order + { + CoreFirst, + ExtensionFirst, + } + + private static IConfiguration Config() => + new ConfigurationBuilder().AddInMemoryCollection(new Dictionary + { + ["Secrets:SqlitePath"] = Path.Combine(Path.GetTempPath(), $"zb-order-{Guid.NewGuid():N}.db"), + ["Secrets:RunMigrationsOnStartup"] = "false", + ["Secrets:MasterKey:Source"] = "Environment", + ["Secrets:MasterKey:EnvVarName"] = "ZB_SECRETS_TEST_KEY", + ["Secrets:SqlServer:ConnectionString"] = "Server=unused;Database=x;", + }).Build(); + + private static ServiceProvider Build(Order order, Action extension) + { + IConfiguration config = Config(); + var services = new ServiceCollection(); + services.AddLogging(); + + if (order == Order.CoreFirst) + { + services.AddZbSecrets(config, "Secrets"); + extension(services, config); + } + else + { + extension(services, config); + services.AddZbSecrets(config, "Secrets"); + } + + return services.BuildServiceProvider(); + } + + [Theory] + [InlineData(Order.CoreFirst)] + [InlineData(Order.ExtensionFirst)] + public void SharedStore_mode_wins_in_either_order(Order order) + { + using ServiceProvider provider = Build(order, (s, c) => s.AddZbSecretsSqlServerStore(c)); + + Assert.IsType(provider.GetRequiredService()); + Assert.IsType(provider.GetRequiredService()); + + // Exactly one store registration is visible, so nothing enumerating the seam can pick up the + // displaced SQLite store either. + Assert.IsType(Assert.Single(provider.GetServices())); + } + + [Theory] + [InlineData(Order.CoreFirst)] + [InlineData(Order.ExtensionFirst)] + public void HubReplication_mode_publishes_through_the_hub_in_either_order(Order order) + { + using ServiceProvider provider = Build(order, (s, c) => s.AddZbSecretsSqlServerReplication(c)); + + Assert.IsType(provider.GetRequiredService()); + Assert.IsType(Assert.Single(provider.GetServices())); + Assert.IsType(provider.GetRequiredService()); + } +} diff --git a/code-reviews/README.md b/code-reviews/README.md index 27fb9fc..18619d6 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -40,7 +40,7 @@ code-reviews/ ## Summary -10 of 10 libraries reviewed. 30 pending findings across all libraries. +10 of 10 libraries reviewed. 29 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 | 11 | +| Medium | 10 | | Low | 19 | -| **Total** | **30** | +| **Total** | **29** | ## 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/6/13 | 19 | 19 | +| [Secrets](Secrets/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/5/13 | 18 | 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 (11) +### Medium (10) | 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-001 | [Secrets](Secrets/findings.md) | Calling `AddZbSecrets` before a SQL-Server extension silently cancels the SQL-Server store / hub replicator | | Secrets-002 | [Secrets](Secrets/findings.md) | The SQL-Server extensions cannot pass the content root, so the pre-host store-path hole reopens in hub mode | | Secrets-003 | [Secrets](Secrets/findings.md) | `${secret:}` expansion is undone by configuration reload | | 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 | diff --git a/code-reviews/Secrets/findings.md b/code-reviews/Secrets/findings.md index cd674ae..b3fbf25 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 | 19 | +| Open findings | 18 | ## Summary @@ -68,7 +68,7 @@ The following were confirmed with throwaway probes in the session scratchpad: Se | Severity | Medium | | Priority | P2 | | Category | Public API surface & compatibility | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.SqlServer/DependencyInjection/SqlServerSecretsServiceCollectionExtensions.cs:53` (also `:54`, `:90`) | **Description** @@ -89,7 +89,7 @@ Make the SQL-Server extensions order-independent: use `Replace` / `RemoveAll` + **Resolution** -_Unresolved._ +Resolved 2026-11-09 in the commit tagged `(scadaproj#30)` (Secrets `0.6.6`, unpublished). `AddZbSecretsSqlServerStore`, `AddZbSecretsSqlServerReplication` and — the same defect class, not in the original finding — `AddZbSecretsAkkaReplication` now call `AddZbSecrets` first and then `RemoveAll` + `AddSingleton` their `ISecretStore` / `ISecretsStoreMigrator` / `ISecretReplicator`, instead of `TryAdd`-ing them beforehand. A later application `AddZbSecrets` only `TryAdd`s, so it cannot displace them; either order yields one `ISecretStore` registration. Regression tests build and resolve the real container in both orders: `Replicator.SqlServer.Tests/DependencyInjection/RegistrationOrderTests` (shared-store + hub) and `Replicator.AkkaDotNet.Tests/DependencyInjection/RegistrationOrderTests` (against a joined single-node cluster). Without the fix the core-first cases resolve `SqliteSecretStore` / `NoOpSecretReplicator`, and the extension-first hub/Akka cases see two `ISecretStore` registrations. ### Secrets-002 — The SQL-Server extensions cannot pass the content root, so the pre-host store-path hole reopens in hub mode