diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Secrets/DeployedSecretsMirror.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Secrets/DeployedSecretsMirror.cs index 7803869b..2070e48b 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Secrets/DeployedSecretsMirror.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Secrets/DeployedSecretsMirror.cs @@ -11,11 +11,12 @@ namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Secrets; /// The outcome of one pass: the secret /// names applied (inserted or updated in this node's own secret store), the names removed /// (deleted because central tombstoned them and this node had previously mirrored them), and -/// a human-readable failure message per live secret that did not decrypt on this node. +/// a human-readable failure message per live secret that could not be applied and/or verified +/// on this node. /// /// Names applied (inserted or updated) this pass. /// Names deleted this pass because they were previously mirrored and are now tombstoned upstream. -/// One message per live secret that failed to decrypt on this node — never includes secret values. +/// One message per live secret that failed to apply and/or decrypt on this node — never includes secret values. public sealed record MirrorResult( IReadOnlyList Applied, IReadOnlyList Removed, IReadOnlyList Failed) { @@ -39,71 +40,132 @@ public sealed record MirrorResult( /// master key. /// /// -/// Apply semantics (). The store applies -/// last-writer-wins on (UpdatedUtc, Revision) — see -/// ZB.MOM.WW.Secrets.Abstractions.SecretLastWriterWins — and a row that is not -/// strictly newer than the local row is silently ignored. Consequence worth knowing: -/// if an operator ever creates an out-of-band secret directly in a site node's local store -/// under the SAME name as a deployed secret, and stamps it with a later UpdatedUtc (or -/// an equal UpdatedUtc and a higher Revision) than the deployed row central last -/// shipped, that out-of-band secret WINS locally — every subsequent reconcile pass's -/// ApplyReplicatedAsync call for that name is a silent no-op until central ships a -/// still-newer revision. This mirror does not attempt to detect or warn about that case (it -/// has no way to distinguish "already applied, unchanged" from "out-of-band override in -/// place" — both look like "nothing to do" from here); deliberately not adding that -/// complexity per the design's scope. +/// Apply semantics and the confirm-then-ledger fix (fix round 1, review Important #1). +/// applies last-writer-wins strictly on +/// (UpdatedUtc, Revision) — see +/// — and a row that is not +/// strictly newer than the local row is silently ignored, so this mirror never trusts the call +/// blindly: every apply is followed by an read-back, and +/// mirrored_secrets is ledgered only when the local row is confirmed LIVE and its +/// ciphertext (Ciphertext + WrappedDek) matches what was just shipped. Two +/// non-ledgered outcomes are possible, both reported as a +/// entry (never as the decrypt-failure message, which would misdiagnose either case) and never +/// overwritten a second way: +/// +/// +/// The local row is a tombstone (this node previously deleted it — e.g. the site +/// stopped referencing it, then referenced it again unchanged at central, so the shipped row's +/// UpdatedUtc is older than the tombstone's). Re-stamping is safe here: neither +/// UpdatedUtc nor Revision is bound into the AEAD associated data — confirmed +/// against ZB.MOM.WW.Secrets.Crypto.AesGcmEnvelopeCipher, whose body layer binds only +/// SecretName.Value and whose wrap layer binds only KekId — so the mirror +/// re-applies the exact shipped ciphertext with UpdatedUtc = max(now, tombstone.UpdatedUtc + +/// 1 tick) and Revision = tombstone.Revision + 1, which beats the tombstone under LWW +/// without touching anything the cipher verifies. If that confirmed re-apply still does not +/// read back live and matching (a concurrent conflict), the row is reported failed instead of +/// silently left broken. +/// +/// +/// The local row is LIVE with different ciphertext — an out-of-band secret created +/// directly on this node under the same name, stamped strictly newer than what central shipped. +/// It wins LWW legitimately. This mirror does NOT overwrite it, does NOT ledger it (so a later +/// central tombstone of the name can never delete someone's out-of-band secret via this +/// mirror), and does NOT run the decrypt-verify against it (verifying would trivially "succeed" +/// against the operator's own value and mask that the deployed revision never took effect). +/// +/// +/// Ledgering always uses the SHIPPED artifact's own revision (not any bumped local revision) +/// so a later pass's "is this ledgered at the shipped revision" check stays correct. +/// +/// +/// Per-row isolation (review Important #2). Each row's apply/verify body runs in its own +/// try/catch; an exception — an unrecognised ContentType from a newer central, a +/// per-row store fault, a malformed name — is caught, reported as a +/// entry naming the secret and the exception type/message (never a value), logged with the same +/// de-spam rule, and does not stop the rest of the pass. Only an exception attributable to the +/// caller's own cancellation (ct.IsCancellationRequested) propagates. +/// +/// +/// Cache invalidation (review Important #3). The concrete resolver +/// (DefaultSecretResolver) caches decrypted plaintext for a TTL and also implements +/// ; every ZB.MOM.WW.Secrets write path invalidates after a +/// write for exactly this reason. This mirror does the same: after every successful apply +/// (including a tombstone re-materialization) or delete, and before the verify step, it calls +/// (injected optionally — null if the +/// host never registered one) so neither the verify nor a script running immediately afterward +/// can observe a stale pre-rotation plaintext for up to the cache TTL. +/// +/// +/// Node-local store assumption (review Minor #5). This mirror assumes the site's +/// is node-local (plain local SQLite, or the gRPC-hub follower half, +/// both of which keep writes local). It is NOT safe to run against a site node whose store is +/// decorated by SQL-Server secrets REPLICATION (Secrets:Replication:Mode=SqlServer) — its +/// DeleteAsync publishes the tombstone to the shared hub, so a site merely un-referencing +/// a secret would delete it fleet-wide. Task 11 rejects that configuration on the Site role; +/// no code change is made here. /// /// /// Resolve semantics and the resolver cache. The trial-resolve verify step goes through /// , which the concrete resolver caches for a TTL -/// (ResolveCacheTtl, 30 s in the shipped implementation). If this node's master key -/// changes (e.g. a KEK rotation) between an earlier successful resolve and a reconcile pass -/// run within the cache TTL, the verify can return the STALE cached plaintext rather than -/// re-decrypting — that is still a successful decrypt of a value under the key that -/// produced the cache entry, which is all this check is trying to prove (this node's master -/// key can decrypt what central most recently shipped), so a cache hit is an acceptable false -/// negative for the "did the key change" question and not a bug in this mirror. +/// (ResolveCacheTtl, 30 s in the shipped implementation). Because every successful apply +/// or delete now invalidates that cache first (see above), the remaining exposure is narrow: a +/// KEK rotation on this node's OWN master key (not a secret rotation at central) landing inside +/// the cache window of an unrelated resolve. That residual case is still an acceptable false +/// negative for the "did the key change" question — the cache hit still proves this node once +/// decrypted a value with a key that produced that cache entry — and is not a bug in this +/// mirror. /// /// -/// Error-log de-spam. A secret that keeps failing to decrypt would otherwise log an -/// Error line every reconcile pass forever. This mirror logs Error only the FIRST time a given -/// (name, revision, kekId) tuple fails on this node; identical repeats log at Debug instead. -/// The moment that same name resolves successfully (a fixed master key, or a newer revision -/// central re-shipped), the remembered tuple is cleared, so a LATER failure — even for the -/// exact same revision/kekId, e.g. a flapping key — logs at Error again. This state is -/// in-memory only (per node process), which matches the ledger's own node-local scope. +/// Error-log de-spam. A secret that keeps failing (to apply OR to decrypt) would +/// otherwise log an Error line every reconcile pass forever. This mirror logs Error only the +/// FIRST time a given failure fingerprint (kind + revision + relevant detail, e.g. KekId for a +/// decrypt failure) occurs for a name on this node; identical repeats log at Debug instead. The +/// moment that same name applies/resolves successfully, the remembered fingerprint is cleared, +/// so a LATER failure — even one with an identical fingerprint — logs at Error again. This state +/// is in-memory only (per node process), which matches the ledger's own node-local scope. /// /// public sealed class DeployedSecretsMirror : BackgroundService { + private const string DeleteActor = "deployed-secrets-mirror"; + private readonly SiteStorageService _storage; private readonly ISecretStore _store; private readonly ISecretResolver _resolver; + private readonly ISecretCacheInvalidator? _invalidator; private readonly IOptions _options; private readonly ILogger _logger; private readonly TimeProvider _time; private readonly SemaphoreSlim _gate = new(1, 1); + private bool _disposed; /// - /// Per-secret-name record of the (Revision, KekId) tuple whose decrypt failure this node - /// has already logged at Error. Cleared for a name the moment it resolves successfully, so - /// a later failure — even an identical one — logs at Error again. In-memory only; not a - /// durable ledger, and deliberately so (it exists purely to de-spam this process's logs). + /// Per-secret-name record of the failure fingerprint this node has already logged at Error + /// (an apply-conflict or a decrypt failure — see the class remarks). Cleared for a name the + /// moment it applies/resolves successfully, so a later failure — even an identical one — + /// logs at Error again. In-memory only; not a durable ledger, and deliberately so (it exists + /// purely to de-spam this process's logs). /// - private readonly Dictionary _lastLoggedFailure = new(StringComparer.Ordinal); + private readonly Dictionary _lastLoggedFailure = new(StringComparer.Ordinal); /// Initializes a new instance of . /// Site-local storage exposing deployed_secrets and the mirrored_secrets ledger. /// This node's own secret store. /// This node's own secret resolver, used to trial-decrypt each deployed secret. + /// + /// Optional cache invalidator for (the concrete + /// DefaultSecretResolver implements this itself; null if the host never + /// registered one, in which case no invalidation is attempted). + /// /// Supplies . /// Logger instance. Never logs secret values — names, revisions, and KEK ids only. - /// Clock driving the reconcile loop's . + /// Clock driving the reconcile loop's and the tombstone-recovery stamp. public DeployedSecretsMirror( SiteStorageService storage, ISecretStore store, ISecretResolver resolver, + ISecretCacheInvalidator? invalidator, IOptions options, ILogger logger, TimeProvider time) @@ -111,6 +173,7 @@ public sealed class DeployedSecretsMirror : BackgroundService _storage = storage ?? throw new ArgumentNullException(nameof(storage)); _store = store ?? throw new ArgumentNullException(nameof(store)); _resolver = resolver ?? throw new ArgumentNullException(nameof(resolver)); + _invalidator = invalidator; _options = options ?? throw new ArgumentNullException(nameof(options)); _logger = logger ?? throw new ArgumentNullException(nameof(logger)); _time = time ?? throw new ArgumentNullException(nameof(time)); @@ -118,10 +181,11 @@ public sealed class DeployedSecretsMirror : BackgroundService /// /// Runs one reconcile pass: applies every live deployed_secrets row into this - /// node's own secret store, deletes rows this node previously mirrored that are now - /// tombstoned upstream, and trial-decrypts every live row to verify it resolves under - /// this node's own master key. Safe to call concurrently with itself (and with the - /// hosted loop) — a single in-flight pass is enforced internally. + /// node's own secret store (confirmed by read-back before ledgering), deletes rows this + /// node previously mirrored that are now tombstoned upstream, and trial-decrypts every + /// live, confirmed-applied row to verify it resolves under this node's own master key. + /// Safe to call concurrently with itself (and with the hosted loop) — a single in-flight + /// pass is enforced internally. /// /// A token to cancel the operation. /// The pass's . @@ -140,38 +204,25 @@ public sealed class DeployedSecretsMirror : BackgroundService foreach (var row in rows) { - if (row.IsRemoved) + try { - if (!mirrored.ContainsKey(row.Name)) + if (row.IsRemoved) { - // Never mirrored by this node (or already un-mirrored) — an - // out-of-band, same-named local secret is left alone. - continue; + await ProcessRemovedRowAsync(row, mirrored, removed, ct).ConfigureAwait(false); + } + else + { + await ProcessLiveRowAsync(row, mirrored, applied, failed, ct).ConfigureAwait(false); } - - await _store.DeleteAsync(new SecretName(row.Name), "deployed-secrets-mirror", ct) - .ConfigureAwait(false); - await _storage.RemoveMirroredSecretAsync(row.Name).ConfigureAwait(false); - removed.Add(row.Name); - _lastLoggedFailure.Remove(row.Name); - _logger.LogInformation( - "Removed deployed secret {SecretName} (no longer referenced by this site)", - row.Name); - continue; } - - if (!mirrored.TryGetValue(row.Name, out var mirroredRevision) - || mirroredRevision != row.Secret.Revision) + catch (Exception ex) when (!ct.IsCancellationRequested) { - await _store.ApplyReplicatedAsync(ToStored(row.Secret), ct).ConfigureAwait(false); - await _storage.SetMirroredSecretAsync(row.Name, row.Secret.Revision).ConfigureAwait(false); - applied.Add(row.Name); - _logger.LogInformation( - "Applied deployed secret {SecretName} r{Revision} (kek {KekId})", - row.Name, row.Secret.Revision, row.Secret.KekId); + ReportFailure( + row.Name, + $"error:{row.Secret.Revision}:{ex.GetType().Name}:{ex.Message}", + $"Secret '{row.Name}' failed to process on this node ({ex.GetType().Name}: {ex.Message})", + failed); } - - await VerifyAsync(row, failed, ct).ConfigureAwait(false); } var result = new MirrorResult(applied, removed, failed); @@ -185,7 +236,7 @@ public sealed class DeployedSecretsMirror : BackgroundService else { _logger.LogDebug( - "Deployed secrets mirror in sync ({Count} secret(s), {FailedCount} failing to decrypt)", + "Deployed secrets mirror in sync ({Count} secret(s), {FailedCount} failing)", rows.Count(r => !r.IsRemoved), failed.Count); } @@ -197,12 +248,146 @@ public sealed class DeployedSecretsMirror : BackgroundService } } + /// + /// Handles a tombstoned deployed_secrets row: deletes it from this node's store, but + /// only if this node had previously ledgered it as mirrored — an out-of-band, same-named + /// local secret this mirror never applied is left alone. + /// + private async Task ProcessRemovedRowAsync( + DeployedSecretRow row, Dictionary mirrored, List removed, CancellationToken ct) + { + if (!mirrored.ContainsKey(row.Name)) + { + return; + } + + await _store.DeleteAsync(new SecretName(row.Name), DeleteActor, ct).ConfigureAwait(false); + _invalidator?.Invalidate(new SecretName(row.Name)); + await _storage.RemoveMirroredSecretAsync(row.Name).ConfigureAwait(false); + removed.Add(row.Name); + _lastLoggedFailure.Remove(row.Name); + _logger.LogInformation( + "Removed deployed secret {SecretName} (no longer referenced by this site)", + row.Name); + } + + /// + /// Handles a live deployed_secrets row: applies it (confirmed by read-back) when not + /// already ledgered at the shipped revision, then verifies it decrypts — unless the apply + /// outcome makes a verify meaningless (an out-of-band local secret won LWW), in which case a + /// single, distinct failure is reported instead. + /// + private async Task ProcessLiveRowAsync( + DeployedSecretRow row, + Dictionary mirrored, + List applied, + List failed, + CancellationToken ct) + { + var name = new SecretName(row.Name); + var alreadyLedgered = + mirrored.TryGetValue(row.Name, out var mirroredRevision) && mirroredRevision == row.Secret.Revision; + + if (!alreadyLedgered) + { + var outcome = await ApplyAndConfirmAsync(row, name, ct).ConfigureAwait(false); + + switch (outcome) + { + case ApplyOutcome.Applied: + await _storage.SetMirroredSecretAsync(row.Name, row.Secret.Revision).ConfigureAwait(false); + _invalidator?.Invalidate(name); + applied.Add(row.Name); + _lastLoggedFailure.Remove(row.Name); + _logger.LogInformation( + "Applied deployed secret {SecretName} r{Revision} (kek {KekId})", + row.Name, row.Secret.Revision, row.Secret.KekId); + break; + + case ApplyOutcome.LocalNewer: + ReportFailure( + row.Name, + $"outofband:{row.Secret.Revision}", + $"Secret '{row.Name}' exists on this node with a newer local value; " + + "the deployed revision was not applied", + failed); + return; // Do not verify — that would decrypt the operator's own value. + + case ApplyOutcome.Unconfirmed: + ReportFailure( + row.Name, + $"unconfirmed:{row.Secret.Revision}", + $"Secret '{row.Name}' could not be confirmed applied on this node " + + "after a tombstone re-apply attempt", + failed); + return; + } + } + + await VerifyAsync(row, failed, ct).ConfigureAwait(false); + } + + /// The confirmed outcome of applying one shipped row into this node's store. See the class remarks. + private enum ApplyOutcome + { + /// The local row is now live and its ciphertext matches what was shipped. + Applied, + + /// The local row is live with DIFFERENT ciphertext — an out-of-band secret that legitimately won LWW. + LocalNewer, + + /// Neither of the above — a tombstone re-apply attempt did not read back as applied. + Unconfirmed, + } + + /// + /// Calls and confirms the outcome by reading + /// the row back — see the class remarks for why this mirror never trusts the call blindly. + /// + private async Task ApplyAndConfirmAsync(DeployedSecretRow row, SecretName name, CancellationToken ct) + { + var shipped = ToStored(row.Secret); + await _store.ApplyReplicatedAsync(shipped, ct).ConfigureAwait(false); + + var local = await _store.GetAsync(name, ct).ConfigureAwait(false); + + if (local is not null && !local.IsDeleted && MatchesShippedCiphertext(local, shipped)) + { + return ApplyOutcome.Applied; + } + + if (local is not null && local.IsDeleted) + { + // Neither UpdatedUtc nor Revision is bound into the AEAD associated data (confirmed + // against AesGcmEnvelopeCipher — only SecretName and KekId are AAD), so re-stamping + // strictly past the tombstone is safe: it changes nothing the cipher verifies. + var nowUtc = _time.GetUtcNow(); + var tombstoneUtcPlusOneTick = local.UpdatedUtc.AddTicks(1); + var bumpedUpdatedUtc = nowUtc > tombstoneUtcPlusOneTick ? nowUtc : tombstoneUtcPlusOneTick; + var bumped = shipped with { UpdatedUtc = bumpedUpdatedUtc, Revision = local.Revision + 1 }; + + await _store.ApplyReplicatedAsync(bumped, ct).ConfigureAwait(false); + var reconfirmed = await _store.GetAsync(name, ct).ConfigureAwait(false); + + return reconfirmed is not null && !reconfirmed.IsDeleted && MatchesShippedCiphertext(reconfirmed, shipped) + ? ApplyOutcome.Applied + : ApplyOutcome.Unconfirmed; + } + + // local is live but its ciphertext differs from what was shipped: an out-of-band value + // with a strictly newer (UpdatedUtc, Revision) won LWW. (local is null only in a + // pathological concurrent-delete race; Unconfirmed covers that too.) + return local is null ? ApplyOutcome.Unconfirmed : ApplyOutcome.LocalNewer; + } + + private static bool MatchesShippedCiphertext(StoredSecret local, StoredSecret shipped) => + local.Ciphertext.AsSpan().SequenceEqual(shipped.Ciphertext) + && local.WrappedDek.AsSpan().SequenceEqual(shipped.WrappedDek); + /// /// Trial-resolves 's secret through to /// verify it decrypts on this node. On failure, appends a message to - /// and logs — Error the first time this exact (name, revision, - /// kekId) has failed since it last resolved successfully on this node, Debug on every - /// repeat (see the class remarks on error-log de-spam). + /// and logs with the shared de-spam rule (see the class remarks). /// private async Task VerifyAsync(DeployedSecretRow row, List failed, CancellationToken ct) { @@ -211,7 +396,7 @@ public sealed class DeployedSecretsMirror : BackgroundService { resolved = await _resolver.GetAsync(new SecretName(row.Name), ct).ConfigureAwait(false) is not null; } - catch (OperationCanceledException) + catch (OperationCanceledException) when (ct.IsCancellationRequested) { throw; } @@ -229,16 +414,25 @@ public sealed class DeployedSecretsMirror : BackgroundService var message = $"Secret '{row.Name}' (kek {row.Secret.KekId}) did not decrypt on this node — " + "the node's master key differs from central's"; + ReportFailure(row.Name, $"decrypt:{row.Secret.Revision}:{row.Secret.KekId}", message, failed); + } + + /// + /// Appends to and logs it — Error the + /// first time is seen for since it + /// last succeeded, Debug on every identical repeat. + /// + private void ReportFailure(string name, string fingerprint, string message, List failed) + { failed.Add(message); - var current = (row.Secret.Revision, row.Secret.KekId); - if (_lastLoggedFailure.TryGetValue(row.Name, out var previous) && previous == current) + if (_lastLoggedFailure.TryGetValue(name, out var previous) && previous == fingerprint) { _logger.LogDebug("{Failure}", message); } else { - _lastLoggedFailure[row.Name] = current; + _lastLoggedFailure[name] = fingerprint; _logger.LogError("{Failure}", message); } } @@ -253,12 +447,15 @@ public sealed class DeployedSecretsMirror : BackgroundService { await ReconcileAsync(stoppingToken).ConfigureAwait(false); } - catch (OperationCanceledException) + catch (OperationCanceledException) when (stoppingToken.IsCancellationRequested) { break; } catch (Exception ex) { + // Covers every other failure, including an OperationCanceledException NOT + // attributable to this service's own shutdown token (e.g. a store/command + // timeout) — that must not silently end the loop (review Minor #4). _logger.LogWarning(ex, "Deployed secrets mirror pass failed; retrying next interval."); } } while (await timer.WaitForNextTickAsync(stoppingToken).ConfigureAwait(false)); @@ -281,4 +478,16 @@ public sealed class DeployedSecretsMirror : BackgroundService CreatedUtc = secret.CreatedUtc, UpdatedUtc = secret.UpdatedUtc, }; + + /// + public override void Dispose() + { + if (!_disposed) + { + _disposed = true; + _gate.Dispose(); + } + + base.Dispose(); + } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Secrets/DeployedSecretsMirrorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Secrets/DeployedSecretsMirrorTests.cs index 824ef930..55692d89 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Secrets/DeployedSecretsMirrorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Secrets/DeployedSecretsMirrorTests.cs @@ -14,6 +14,13 @@ namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.Secrets; /// Task 9, site-secrets artifact deploy: — per-node apply /// + decrypt-verify of deployed_secrets into this node's own secret store. /// +/// +/// Fix round 1 (review task-9-review.md): now honours the +/// real SqliteSecretStore semantics — ApplyReplicatedAsync is strict +/// last-writer-wins via , and DeleteAsync +/// tombstones rather than hard-deleting — so tests exercise the same conflict shapes the real +/// store can produce, not an idealized "always overwrite" double. +/// public class DeployedSecretsMirrorTests : IDisposable { private readonly TestLocalDb _localDb; @@ -35,8 +42,11 @@ public class DeployedSecretsMirrorTests : IDisposable [Fact] public async Task Reconcile_Applies_Verifies_AndOnlyDeletesWhatItMirrored() { - var (mirror, storage, store, _) = await CreateAsync(); - store.Seed("out-of-band"); + var (mirror, storage, store, _, _) = await CreateAsync(); + // Seeded OLDER than what will be shipped, so the shipped row legitimately wins LWW + // and is applied — the strictly-NEWER out-of-band case is covered separately by + // Reconcile_OutOfBandNewerSecret_NotOverwritten_NotLedgered_AndSurvivesLaterRemoval. + store.Seed("out-of-band", updatedUtc: new DateTimeOffset(2026, 9, 27, 0, 0, 0, TimeSpan.Zero)); await storage.UpsertDeployedSecretsAsync([Art("a", 1), Art("out-of-band", 1)], "d1"); var r1 = await mirror.ReconcileAsync(CancellationToken.None); @@ -45,8 +55,9 @@ public class DeployedSecretsMirrorTests : IDisposable Assert.Empty(r1.Failed); await storage.UpsertDeployedSecretsAsync([], "d2"); - // Simulate: OUT-OF-BAND was never actually mirrored by this node (an operator secret - // that happens to share the name) by clearing its ledger row directly. + // Simulate: "out-of-band" was never actually mirrored by this node (an operator secret + // that happens to share the name) by clearing its ledger row directly — a defensive + // check independent of the natural not-ledgered path exercised below. await storage.RemoveMirroredSecretAsync("out-of-band"); var r2 = await mirror.ReconcileAsync(CancellationToken.None); @@ -54,12 +65,86 @@ public class DeployedSecretsMirrorTests : IDisposable Assert.Equal(["a"], r2.Removed); Assert.True(store.Contains("out-of-band")); Assert.False(store.Contains("a")); + Assert.True(store.IsTombstoned("a")); + } + + [Fact] + public async Task Reconcile_OutOfBandNewerSecret_NotOverwritten_NotLedgered_AndSurvivesLaterRemoval() + { + var (mirror, storage, store, _, invalidator) = await CreateAsync(); + var operatorCiphertext = new byte[] { 200, 201, 202 }; + // Strictly newer than Art()'s fixed UpdatedUtc (2026-09-28T01:00:00Z) — legitimately + // wins LWW, exactly as the real store would order it. + store.Seed("a", + updatedUtc: new DateTimeOffset(2026, 9, 28, 2, 0, 0, TimeSpan.Zero), + revision: 5, + ciphertext: operatorCiphertext); + + await storage.UpsertDeployedSecretsAsync([Art("a", 1)], "d1"); + var r1 = await mirror.ReconcileAsync(CancellationToken.None); + + Assert.Empty(r1.Applied); + var message = Assert.Single(r1.Failed); + Assert.Contains("'a'", message); + Assert.Contains("newer local value", message); + Assert.DoesNotContain("did not decrypt", message); // distinct from the master-key message + Assert.Equal(operatorCiphertext, store.Rows["a"].Ciphertext); // untouched + Assert.Empty(invalidator.Invalidated); // never applied -> never invalidated + + // Never ledgered, so a later central tombstone must NOT delete the operator's secret. + await storage.UpsertDeployedSecretsAsync([], "d2"); + var r2 = await mirror.ReconcileAsync(CancellationToken.None); + + Assert.Empty(r2.Removed); + Assert.True(store.Contains("a")); + Assert.Equal(operatorCiphertext, store.Rows["a"].Ciphertext); + } + + [Fact] + public async Task Reconcile_RemoveThenReAdd_TombstoneIsRecovered_ReappliesAndVerifies() + { + var time = new FakeTimeProvider(new DateTimeOffset(2026, 9, 28, 6, 0, 0, TimeSpan.Zero)); + var (mirror, storage, store, _, invalidator) = await CreateAsync(time: time); + await storage.UpsertDeployedSecretsAsync([Art("a", 1)], "d1"); + var firstApply = await mirror.ReconcileAsync(CancellationToken.None); + Assert.Equal(["a"], firstApply.Applied); + + // Site stops referencing it: central tombstones the row, this node deletes it. The + // tombstone is stamped LATER than the originally-shipped UpdatedUtc (01:00). + store.Now = new DateTimeOffset(2026, 9, 28, 12, 0, 0, TimeSpan.Zero); + await storage.UpsertDeployedSecretsAsync([], "d2"); + var removePass = await mirror.ReconcileAsync(CancellationToken.None); + Assert.Equal(["a"], removePass.Removed); + Assert.True(store.IsTombstoned("a")); + invalidator.Invalidated.Clear(); + + // Site references it again — central ships the SAME (unchanged, older-stamped) + // secret. A naive ApplyReplicatedAsync call is a no-op against the newer tombstone; + // the mirror must detect and recover this. + time.Now = new DateTimeOffset(2026, 9, 28, 18, 0, 0, TimeSpan.Zero); // later than the tombstone + await storage.UpsertDeployedSecretsAsync([Art("a", 1)], "d3"); + var reAddPass = await mirror.ReconcileAsync(CancellationToken.None); + + Assert.Equal(["a"], reAddPass.Applied); + Assert.Empty(reAddPass.Failed); + Assert.True(store.Contains("a")); + Assert.Equal(new byte[] { 1, 2, 3, 4, 5 }, store.Rows["a"].Ciphertext); // Art()'s ciphertext, re-materialized + Assert.Contains("a", invalidator.Invalidated); + + // Ledgering used the SHIPPED artifact's own revision, not the bumped local one, so a + // later pass with the same shipped row is a true no-op. + var thirdPass = await mirror.ReconcileAsync(CancellationToken.None); + Assert.Empty(thirdPass.Applied); + Assert.False(thirdPass.Changed); + + var mirrored = await storage.GetMirroredSecretsAsync(); + Assert.Equal(1, mirrored.Single(m => m.Name == "a").Revision); } [Fact] public async Task Reconcile_ReAppliesOnlyWhenRevisionChanges() { - var (mirror, storage, store, _) = await CreateAsync(); + var (mirror, storage, store, _, _) = await CreateAsync(); await storage.UpsertDeployedSecretsAsync([Art("a", 1)], "d1"); var r1 = await mirror.ReconcileAsync(CancellationToken.None); @@ -72,18 +157,18 @@ public class DeployedSecretsMirrorTests : IDisposable await storage.UpsertDeployedSecretsAsync([Art("a", 2)], "d2"); var r3 = await mirror.ReconcileAsync(CancellationToken.None); Assert.Equal(["a"], r3.Applied); - Assert.Equal(2, store.LastApplied["a"].Revision); + Assert.Equal(2, store.Rows["a"].Revision); } [Fact] public async Task Reconcile_AppliesThroughApplyReplicatedAsync_MappedFromSecretArtifact() { - var (mirror, storage, store, _) = await CreateAsync(); + var (mirror, storage, store, _, _) = await CreateAsync(); await storage.UpsertDeployedSecretsAsync([Art("a", 1, kek: "kek-central")], "d1"); await mirror.ReconcileAsync(CancellationToken.None); - var applied = store.LastApplied["a"]; + var applied = store.Rows["a"]; Assert.Equal("kek-central", applied.KekId); Assert.Equal(1, applied.Revision); Assert.Equal(SecretContentType.Text, applied.ContentType); @@ -96,34 +181,119 @@ public class DeployedSecretsMirrorTests : IDisposable [InlineData(SecretContentType.BinaryBase64)] public async Task Reconcile_RoundTripsEveryContentType(SecretContentType contentType) { - var (mirror, storage, store, _) = await CreateAsync(); + var (mirror, storage, store, _, _) = await CreateAsync(); await storage.UpsertDeployedSecretsAsync( [Art("a", 1, contentType: contentType.ToString())], "d1"); await mirror.ReconcileAsync(CancellationToken.None); - Assert.Equal(contentType, store.LastApplied["a"].ContentType); + Assert.Equal(contentType, store.Rows["a"].ContentType); + } + + // ── Per-row isolation (ruling 2) ── + + [Fact] + public async Task Reconcile_OneRowThrows_OthersStillApply() + { + var (mirror, storage, store, _, _) = await CreateAsync(); + // "bad" carries an unrecognised ContentType (as a newer central under version skew + // might ship), so ToStored's Enum.Parse throws for that row only. + await storage.UpsertDeployedSecretsAsync( + [Art("bad", 1, contentType: "NotARealContentType"), Art("good", 1)], "d1"); + + var r = await mirror.ReconcileAsync(CancellationToken.None); + + Assert.Equal(["good"], r.Applied); + var message = Assert.Single(r.Failed); + Assert.Contains("'bad'", message); + Assert.True(store.Contains("good")); + Assert.False(store.Contains("bad")); + } + + [Fact] + public async Task Reconcile_OneRowThrows_FailureIsDeSpammedLikeAnyOtherFailure() + { + var logger = new CapturingLogger(); + var (mirror, storage, _, _, _) = await CreateAsync(logger); + await storage.UpsertDeployedSecretsAsync([Art("bad", 1, contentType: "NotARealContentType")], "d1"); + + await mirror.ReconcileAsync(CancellationToken.None); + await mirror.ReconcileAsync(CancellationToken.None); + + var levels = logger.Entries + .Where(e => e.Message.Contains("'bad'") && e.Message.Contains("failed to process")) + .Select(e => e.Level) + .ToList(); + Assert.Equal([LogLevel.Error, LogLevel.Debug], levels); + } + + // ── Cache invalidation (ruling 3) ── + + [Fact] + public async Task Reconcile_SuccessfulApply_InvalidatesCache_BeforeVerify() + { + var (mirror, storage, _, _, invalidator) = await CreateAsync(); + await storage.UpsertDeployedSecretsAsync([Art("a", 1)], "d1"); + + await mirror.ReconcileAsync(CancellationToken.None); + + Assert.Contains("a", invalidator.Invalidated); + } + + [Fact] + public async Task Reconcile_Removal_InvalidatesCache() + { + var (mirror, storage, _, _, invalidator) = await CreateAsync(); + await storage.UpsertDeployedSecretsAsync([Art("a", 1)], "d1"); + await mirror.ReconcileAsync(CancellationToken.None); + invalidator.Invalidated.Clear(); + + await storage.UpsertDeployedSecretsAsync([], "d2"); + await mirror.ReconcileAsync(CancellationToken.None); + + Assert.Contains("a", invalidator.Invalidated); + } + + [Fact] + public async Task Reconcile_WithNoInvalidatorRegistered_StillApplies() + { + var storage = new SiteStorageService(_localDb.Db, NullLogger.Instance); + await storage.InitializeAsync(); + var mirror = new DeployedSecretsMirror( + storage, + new FakeSecretStore(), + new FakeSecretResolver(), + invalidator: null, + Options.Create(new SiteRuntimeOptions { DeployedSecretsMirrorInterval = TimeSpan.FromSeconds(15) }), + NullLogger.Instance, + TimeProvider.System); + + await storage.UpsertDeployedSecretsAsync([Art("a", 1)], "d1"); + var r = await mirror.ReconcileAsync(CancellationToken.None); + + Assert.Equal(["a"], r.Applied); } // ── Decrypt verification ── [Fact] - public async Task Reconcile_DecryptFailure_IsReportedWithKekId() + public async Task Reconcile_DecryptFailure_ExceptionFromResolver_IsReportedWithKekId() { - var (mirror, storage, _, resolver) = await CreateAsync(); - resolver.FailFor("a"); + var (mirror, storage, _, resolver, _) = await CreateAsync(); + resolver.ThrowFor("a"); await storage.UpsertDeployedSecretsAsync([Art("a", 1, kek: "kek-central")], "d1"); var r = await mirror.ReconcileAsync(CancellationToken.None); - Assert.Contains("kek-central", Assert.Single(r.Failed)); - Assert.Contains("'a'", Assert.Single(r.Failed)); + var message = Assert.Single(r.Failed); + Assert.Contains("kek-central", message); + Assert.Contains("'a'", message); } [Fact] public async Task Reconcile_DecryptFailure_NullResolve_IsAlsoReported() { - var (mirror, storage, _, resolver) = await CreateAsync(); + var (mirror, storage, _, resolver, _) = await CreateAsync(); resolver.ReturnNullFor("a"); await storage.UpsertDeployedSecretsAsync([Art("a", 1)], "d1"); @@ -132,22 +302,10 @@ public class DeployedSecretsMirrorTests : IDisposable Assert.Single(r.Failed); } - [Fact] - public async Task Reconcile_DecryptFailure_ExceptionFromResolver_IsAlsoReported() - { - var (mirror, storage, _, resolver) = await CreateAsync(); - resolver.ThrowFor("a"); - - await storage.UpsertDeployedSecretsAsync([Art("a", 1)], "d1"); - var r = await mirror.ReconcileAsync(CancellationToken.None); - - Assert.Single(r.Failed); - } - [Fact] public async Task Reconcile_VerifiesEveryPass_EvenWhenAlreadyApplied() { - var (mirror, storage, _, resolver) = await CreateAsync(); + var (mirror, storage, _, resolver, _) = await CreateAsync(); await storage.UpsertDeployedSecretsAsync([Art("a", 1)], "d1"); var r1 = await mirror.ReconcileAsync(CancellationToken.None); @@ -156,20 +314,20 @@ public class DeployedSecretsMirrorTests : IDisposable // Master key "changes" (or a KEK rotation invalidates the local unwrap) between // passes — even though nothing about deployed_secrets changed, the next reconcile // must still catch the failure because it re-verifies unconditionally. - resolver.FailFor("a"); + resolver.ThrowFor("a"); var r2 = await mirror.ReconcileAsync(CancellationToken.None); Assert.Single(r2.Failed); Assert.Empty(r2.Applied); // unchanged revision — not re-applied, only re-verified } - // ── Error-log de-spam (ruling 1) ── + // ── Error-log de-spam (ruling 1 of the original task, unaffected by fix round 1) ── [Fact] public async Task Reconcile_DecryptFailure_LogsErrorOnlyFirstTime_ThenDebug_ThenErrorAgainAfterASuccess() { var logger = new CapturingLogger(); - var (mirror, storage, _, resolver) = await CreateAsync(logger); - resolver.FailFor("a"); + var (mirror, storage, _, resolver, _) = await CreateAsync(logger); + resolver.ThrowFor("a"); await storage.UpsertDeployedSecretsAsync([Art("a", 1, kek: "kek-1")], "d1"); await mirror.ReconcileAsync(CancellationToken.None); // 1st failure: Error @@ -190,7 +348,7 @@ public class DeployedSecretsMirrorTests : IDisposable Assert.Empty(recovered.Failed); // Fails again — must log at Error again, not Debug, since it succeeded in between. - resolver.FailFor("a"); + resolver.ThrowFor("a"); await mirror.ReconcileAsync(CancellationToken.None); var failureEntriesAfterRecovery = logger.Entries @@ -204,8 +362,8 @@ public class DeployedSecretsMirrorTests : IDisposable public async Task Reconcile_DecryptFailure_NewRevision_LogsErrorAgain_EvenWithoutASuccessInBetween() { var logger = new CapturingLogger(); - var (mirror, storage, _, resolver) = await CreateAsync(logger); - resolver.FailFor("a"); + var (mirror, storage, _, resolver, _) = await CreateAsync(logger); + resolver.ThrowFor("a"); await storage.UpsertDeployedSecretsAsync([Art("a", 1, kek: "kek-1")], "d1"); await mirror.ReconcileAsync(CancellationToken.None); // Error (rev 1) @@ -222,12 +380,63 @@ public class DeployedSecretsMirrorTests : IDisposable Assert.Equal([LogLevel.Error, LogLevel.Debug, LogLevel.Error], levels); } + // ── Cancellation + concurrency (ruling 6) ── + + [Fact] + public async Task ReconcileAsync_PreCancelledToken_ThrowsOperationCanceled() + { + var (mirror, _, _, _, _) = await CreateAsync(); + using var cts = new CancellationTokenSource(); + cts.Cancel(); + + await Assert.ThrowsAnyAsync( + () => mirror.ReconcileAsync(cts.Token)); + } + + [Fact] + public async Task ReconcileAsync_ConcurrentCallers_BothCompleteWithoutDoubleApply() + { + var (mirror, storage, store, _, _) = await CreateAsync(); + await storage.UpsertDeployedSecretsAsync([Art("a", 1)], "d1"); + + var t1 = mirror.ReconcileAsync(CancellationToken.None); + var t2 = mirror.ReconcileAsync(CancellationToken.None); + var results = await Task.WhenAll(t1, t2); + + // The gate serializes the two passes: whichever runs first applies it, the other + // sees it already ledgered at the shipped revision and applies nothing. + Assert.Equal(1, results.Sum(r => r.Applied.Count)); + Assert.True(store.Contains("a")); + + var mirrored = await storage.GetMirroredSecretsAsync(); + Assert.Equal(1, mirrored.Single(m => m.Name == "a").Revision); + } + + // ── Dispose (ruling 6) ── + + [Fact] + public void Dispose_DoesNotThrow_AndIsIdempotent() + { + var storage = new SiteStorageService(_localDb.Db, NullLogger.Instance); + var mirror = new DeployedSecretsMirror( + storage, + new FakeSecretStore(), + new FakeSecretResolver(), + invalidator: null, + Options.Create(new SiteRuntimeOptions()), + NullLogger.Instance, + TimeProvider.System); + + mirror.Dispose(); + mirror.Dispose(); + } + // ── Hosted loop ── [Fact] public async Task ExecuteAsync_RunsOnceAtStart_AndAgainEveryInterval() { - var (mirror, storage, store, resolver) = await CreateAsync( + var (mirror, storage, store, resolver, _) = await CreateAsync( interval: TimeSpan.FromMilliseconds(30)); await storage.UpsertDeployedSecretsAsync([Art("a", 1)], "d1"); @@ -250,10 +459,10 @@ public class DeployedSecretsMirrorTests : IDisposable } [Fact] - public async Task ExecuteAsync_ReconcileExceptionIsLoggedAndLoopContinues() + public async Task ExecuteAsync_PerRowFailure_IsIsolated_AndLoopRecoversOnceCleared() { var logger = new CapturingLogger(); - var (mirror, storage, store, _) = await CreateAsync( + var (mirror, storage, store, _, _) = await CreateAsync( logger, interval: TimeSpan.FromMilliseconds(30)); await storage.UpsertDeployedSecretsAsync([Art("a", 1)], "d1"); store.ThrowOnApply = true; @@ -261,8 +470,10 @@ public class DeployedSecretsMirrorTests : IDisposable await mirror.StartAsync(CancellationToken.None); try { + // A per-row store fault is isolated (ruling 2): it never escapes ReconcileAsync, + // so it shows up as a row-level Error log, not the pass-level Warning. await WaitUntilAsync(() => logger.Entries.Any( - e => e.Level == LogLevel.Warning && e.Message.Contains("failed"))); + e => e.Level == LogLevel.Error && e.Message.Contains("failed to process"))); // The loop must still be alive after the failure — force success and confirm a // later pass applies the secret. @@ -278,15 +489,22 @@ public class DeployedSecretsMirrorTests : IDisposable // ── Harness ── - private async Task<(DeployedSecretsMirror Mirror, SiteStorageService Storage, FakeSecretStore Store, FakeSecretResolver Resolver)> CreateAsync( + private async Task<( + DeployedSecretsMirror Mirror, + SiteStorageService Storage, + FakeSecretStore Store, + FakeSecretResolver Resolver, + FakeSecretCacheInvalidator Invalidator)> CreateAsync( CapturingLogger? logger = null, - TimeSpan? interval = null) + TimeSpan? interval = null, + TimeProvider? time = null) { var storage = new SiteStorageService(_localDb.Db, NullLogger.Instance); await storage.InitializeAsync(); var store = new FakeSecretStore(); var resolver = new FakeSecretResolver(); + var invalidator = new FakeSecretCacheInvalidator(); var options = Options.Create(new SiteRuntimeOptions { DeployedSecretsMirrorInterval = interval ?? TimeSpan.FromSeconds(15), @@ -296,11 +514,12 @@ public class DeployedSecretsMirrorTests : IDisposable storage, store, resolver, + invalidator, options, (ILogger?)logger ?? NullLogger.Instance, - TimeProvider.System); + time ?? TimeProvider.System); - return (mirror, storage, store, resolver); + return (mirror, storage, store, resolver, invalidator); } private static SecretArtifact Art( @@ -345,38 +564,68 @@ public class DeployedSecretsMirrorTests : IDisposable => Entries.Enqueue((logLevel, formatter(state, exception))); } + /// Minimal controllable — mirrors the pattern used by ApiMethodCacheTests. + private sealed class FakeTimeProvider : TimeProvider + { + public DateTimeOffset Now { get; set; } + public FakeTimeProvider(DateTimeOffset start) => Now = start; + public override DateTimeOffset GetUtcNow() => Now; + } + + /// Records every call by secret name. + private sealed class FakeSecretCacheInvalidator : ISecretCacheInvalidator + { + public List Invalidated { get; } = new(); + public void Invalidate(SecretName name) => Invalidated.Add(name.Value); + } + /// - /// In-memory recording / - /// calls, for asserting what the mirror applied/removed without - /// a real ZB.MOM.WW.Secrets store. + /// In-memory honouring the REAL SqliteSecretStore + /// semantics (fix round 1, review Important #1): is + /// strict last-writer-wins via and applies + /// verbatim (no revision bump); tombstones in place (bumps + /// revision, stamps ) rather than hard-deleting. /// private sealed class FakeSecretStore : ISecretStore { private readonly Dictionary _rows = new(StringComparer.Ordinal); - public IReadOnlyDictionary LastApplied => _rows; + /// + /// Controllable "now" used to stamp a tombstone — + /// deterministic, not wall-clock, so tests can control ordering against a shipped + /// artifact's fixed UpdatedUtc. + /// + public DateTimeOffset Now { get; set; } = new DateTimeOffset(2026, 9, 28, 12, 0, 0, TimeSpan.Zero); /// Set to make the next call throw, simulating a store fault. public bool ThrowOnApply { get; set; } - /// Seeds a row directly (bypassing ApplyReplicatedAsync) — an out-of-band secret this node never mirrored itself. - public void Seed(string name) => _rows[name] = new StoredSecret - { - Name = new SecretName(name), - ContentType = SecretContentType.Text, - Ciphertext = [9], - Nonce = [9], - Tag = [9], - WrappedDek = [9], - WrapNonce = [9], - WrapTag = [9], - KekId = "kek-out-of-band", - Revision = 1, - CreatedUtc = DateTimeOffset.UtcNow, - UpdatedUtc = DateTimeOffset.UtcNow, - }; + /// Every row currently held, including tombstones, for direct inspection. + public IReadOnlyDictionary Rows => _rows; - public bool Contains(string name) => _rows.ContainsKey(name); + /// Seeds a LIVE row directly (bypassing ApplyReplicatedAsync/LWW) — an out-of-band secret. + public void Seed(string name, DateTimeOffset updatedUtc, long revision = 1, byte[]? ciphertext = null) => + _rows[name] = new StoredSecret + { + Name = new SecretName(name), + ContentType = SecretContentType.Text, + Ciphertext = ciphertext ?? [9], + Nonce = [9], + Tag = [9], + WrappedDek = [9], + WrapNonce = [9], + WrapTag = [9], + KekId = "kek-out-of-band", + Revision = revision, + CreatedUtc = updatedUtc, + UpdatedUtc = updatedUtc, + }; + + /// True if the row is present and NOT tombstoned. + public bool Contains(string name) => _rows.TryGetValue(name, out var row) && !row.IsDeleted; + + /// True if the row is present as a tombstone. + public bool IsTombstoned(string name) => _rows.TryGetValue(name, out var row) && row.IsDeleted; public Task GetAsync(SecretName name, CancellationToken ct) => Task.FromResult(_rows.TryGetValue(name.Value, out var row) ? row : null); @@ -387,8 +636,24 @@ public class DeployedSecretsMirrorTests : IDisposable return Task.CompletedTask; } - public Task DeleteAsync(SecretName name, string? actor, CancellationToken ct) => - Task.FromResult(_rows.Remove(name.Value)); + /// Tombstones (does not hard-delete) — matches SqliteSecretStore.DeleteAsync. + public Task DeleteAsync(SecretName name, string? actor, CancellationToken ct) + { + if (!_rows.TryGetValue(name.Value, out var existing) || existing.IsDeleted) + { + return Task.FromResult(false); + } + + _rows[name.Value] = existing with + { + IsDeleted = true, + DeletedUtc = Now, + UpdatedUtc = Now, + Revision = existing.Revision + 1, + UpdatedBy = actor, + }; + return Task.FromResult(true); + } public Task> ListAsync(bool includeDeleted, CancellationToken ct) => Task.FromResult>([]); @@ -396,6 +661,7 @@ public class DeployedSecretsMirrorTests : IDisposable public Task> GetManifestAsync(CancellationToken ct) => Task.FromResult>([]); + /// Strict LWW on (UpdatedUtc, Revision) via ; applied verbatim — matches SqliteSecretStore. public Task ApplyReplicatedAsync(StoredSecret row, CancellationToken ct) { if (ThrowOnApply) @@ -403,9 +669,12 @@ public class DeployedSecretsMirrorTests : IDisposable throw new InvalidOperationException("simulated store fault"); } - // Test double only — real store applies last-writer-wins; here every apply - // just overwrites, which is fine since these tests never seed a conflicting - // higher local revision through this path. + if (_rows.TryGetValue(row.Name.Value, out var existing) + && !SecretLastWriterWins.IsNewer(row.UpdatedUtc, row.Revision, existing.UpdatedUtc, existing.Revision)) + { + return Task.CompletedTask; // no-op, matches real semantics + } + _rows[row.Name.Value] = row; return Task.CompletedTask; } @@ -423,9 +692,12 @@ public class DeployedSecretsMirrorTests : IDisposable public int CallCount; - public void FailFor(string name) => _nullReturning.Add(name); + /// Row absent/tombstoned path — DefaultSecretResolver.GetAsync returns null. public void ReturnNullFor(string name) => _nullReturning.Add(name); + + /// Decryption/integrity failure path — DefaultSecretResolver.GetAsync rethrows SecretDecryptionException. public void ThrowFor(string name) => _throwing.Add(name); + public void ClearFailures() { _nullReturning.Clear();