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
This commit is contained in:
Joseph Doherty
2026-11-09 14:47:01 -05:00
parent 1786bab0a0
commit 015c556ced
12 changed files with 263 additions and 31 deletions
+14
View File
@@ -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
+2
View File
@@ -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.
+1 -1
View File
@@ -5,7 +5,7 @@
<Nullable>enable</Nullable>
<ImplicitUsings>enable</ImplicitUsings>
<LangVersion>latest</LangVersion>
<Version>0.6.5</Version>
<Version>0.6.6</Version>
<ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
<PackageReadmeFile>README.md</PackageReadmeFile>
</PropertyGroup>
+4
View File
@@ -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.
@@ -82,20 +82,23 @@ public static class AkkaSecretsServiceCollectionExtensions
new DeferredSecretCacheInvalidator(sp.GetService<ISecretCacheInvalidator>),
sp.GetRequiredService<IOptions<AkkaSecretsReplicationOptions>>().Value));
// MUST be registered BEFORE AddZbSecrets. That call does
// TryAddSingleton<ISecretReplicator, NoOpSecretReplicator>(), 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<ISecretReplicator>(sp =>
new AkkaSecretReplicator(sp.GetRequiredService<SecretReplicationActorProvider>().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<ISecretReplicator, NoOpSecretReplicator>(); 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<ISecretReplicator>();
services.AddSingleton<ISecretReplicator>(sp =>
new AkkaSecretReplicator(sp.GetRequiredService<SecretReplicationActorProvider>().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<ISecretStore>();
services.AddSingleton<ISecretStore>(sp => new ReplicatingSecretStore(
sp.GetRequiredService<SqliteSecretStore>(),
sp.GetRequiredService<ISecretReplicator>(),
@@ -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<ISecretStore>(sp => sp.GetRequiredService<SqlServerSecretStore>());
services.TryAddSingleton<ISecretsStoreMigrator>(sp =>
sp.GetRequiredService<SqlServerSecretsStoreMigrator>());
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<ISecretStore>();
services.AddSingleton<ISecretStore>(sp => sp.GetRequiredService<SqlServerSecretStore>());
services.RemoveAll<ISecretsStoreMigrator>();
services.AddSingleton<ISecretsStoreMigrator>(sp =>
sp.GetRequiredService<SqlServerSecretsStoreMigrator>());
return services;
}
@@ -87,15 +92,22 @@ public static class SqlServerSecretsServiceCollectionExtensions
services.TryAddSingleton<ISecretReplicationHub>(sp => sp.GetRequiredService<SqlServerSecretStore>());
services.TryAddSingleton<ISecretReplicator>(sp =>
new SqlServerSecretReplicator(sp.GetRequiredService<ISecretReplicationHub>()));
// 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<ISecretReplicator>();
services.AddSingleton<ISecretReplicator>(sp =>
new SqlServerSecretReplicator(sp.GetRequiredService<ISecretReplicationHub>()));
// 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<ISecretStore>();
services.AddSingleton<ISecretStore>(sp => new ReplicatingSecretStore(
sp.GetRequiredService<Secrets.Sqlite.SqliteSecretStore>(),
sp.GetRequiredService<ISecretReplicator>(),
@@ -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<SecretsSqliteConnectionFactory>()));
@@ -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;
/// <summary>
/// scadaproj#30 (Secrets-001), Akka half: the package must bind its own replicator and decorator
/// whether the application calls <c>AddZbSecrets</c> before or after it. Until 0.6.6 the package
/// TryAdd-ed its replicator ahead of its internal <c>AddZbSecrets</c>, which fixed the 0.2.0 order
/// bug only for the package's own call — an application that called <c>AddZbSecrets</c> FIRST still
/// got <c>NoOpSecretReplicator</c>. Builds the real container against a joined cluster and resolves.
/// </summary>
public sealed class RegistrationOrderTests(SingleNodeClusterFixture cluster)
: IClassFixture<SingleNodeClusterFixture>
{
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<string, string?>
{
["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<AkkaSecretReplicator>(provider.GetRequiredService<ISecretReplicator>());
Assert.IsType<ReplicatingSecretStore>(Assert.Single(provider.GetServices<ISecretStore>()));
}
}
@@ -0,0 +1,49 @@
using Akka.Actor;
using Akka.Cluster;
using Akka.Configuration;
namespace ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests.DependencyInjection;
/// <summary>
/// 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 <c>Secrets:Replication:ActorName</c>, since actor names are unique per system.
/// </summary>
public sealed class SingleNodeClusterFixture : IAsyncLifetime
{
/// <summary>The joined, Up actor system.</summary>
public ActorSystem System { get; private set; } = null!;
/// <inheritdoc />
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);
}
}
/// <inheritdoc />
public async Task DisposeAsync() => await System.Terminate();
}
@@ -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;
/// <summary>
/// scadaproj#30 (Secrets-001): the SQL-Server extensions must produce the same graph whether the
/// application calls <c>AddZbSecrets</c> before or after them.
/// </summary>
/// <remarks>
/// Before the fix both modes put their registrations in with <c>TryAdd</c> ahead of their own
/// internal <c>AddZbSecrets</c> call, so an application that had ALREADY called <c>AddZbSecrets</c>
/// — the order the README teaches, and the only way to pass <c>contentRootPath</c> — 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.
/// </remarks>
public sealed class RegistrationOrderTests
{
public enum Order
{
CoreFirst,
ExtensionFirst,
}
private static IConfiguration Config() =>
new ConfigurationBuilder().AddInMemoryCollection(new Dictionary<string, string?>
{
["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<IServiceCollection, IConfiguration> 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<SqlServerSecretStore>(provider.GetRequiredService<ISecretStore>());
Assert.IsType<SqlServerSecretsStoreMigrator>(provider.GetRequiredService<ISecretsStoreMigrator>());
// Exactly one store registration is visible, so nothing enumerating the seam can pick up the
// displaced SQLite store either.
Assert.IsType<SqlServerSecretStore>(Assert.Single(provider.GetServices<ISecretStore>()));
}
[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<SqlServerSecretReplicator>(provider.GetRequiredService<ISecretReplicator>());
Assert.IsType<ReplicatingSecretStore>(Assert.Single(provider.GetServices<ISecretStore>()));
Assert.IsType<SqliteSecretsStoreMigrator>(provider.GetRequiredService<ISecretsStoreMigrator>());
}
}
+5 -6
View File
@@ -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 |
+3 -3
View File
@@ -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