fix(site-secrets): contain secret-store outages per site; audit removal-only deploys

Secret-store outage (final review M5): SiteSecretsCollector returns a store read failure as SiteSecretsCollection.StoreError instead of throwing. A deploy now fails only the sites that reference a secret (nothing sent, not stamped, DeploySiteSecrets row with the error), and ComputeCurrentArtifactHashesAsync omits only those sites (they read Out of date) instead of the exception aborting the whole sweep and the fleet-wide pending-updates check. Removal-only deploys: the site reply gains SecretsRemoved (proto field 7, names only) - the deployed_secrets rows the deploy soft-removed. Central carries it on SiteArtifactResult and writes the DeploySiteSecrets row with a removed list whenever it is non-empty, so dropping a site's last secret: reference is audited centrally rather than only in site logs.
2026-09-28 21:23:52 -04:00
parent ef03a0dca7
commit 29049c4ef6
15 changed files with 875 additions and 423 deletions
@@ -82,4 +82,23 @@ public class SiteCommandDtoMapperSecretsTests
        var r = new ArtifactDeploymentResponse("dep", "site-a", true, null, DateTimeOffset.UtcNow, applied);
        Assert.Equal(applied, SiteCommandDtoMapper.FromProto(SiteCommandDtoMapper.ToProto(r)).SecretsApplied);
    }
+
+    [Fact]
+    public void ArtifactResponse_SecretsRemoved_RoundTrips()
+    {
+        var r = new ArtifactDeploymentResponse(
+            "dep", "site-a", true, null, DateTimeOffset.UtcNow, 0, ["old-1", "old-2"]);
+        Assert.Equal(["old-1", "old-2"], SiteCommandDtoMapper.FromProto(SiteCommandDtoMapper.ToProto(r)).SecretsRemoved!);
+    }
+
+    [Fact]
+    public void ArtifactResponse_NoSecretsRemoved_IsNullOnBothSides()
+    {
+        // Absent on the wire (an old site, or nothing removed) must read back as null.
+        var none = new ArtifactDeploymentResponse("dep", "site-a", true, null, DateTimeOffset.UtcNow, 1);
+        Assert.Null(SiteCommandDtoMapper.FromProto(SiteCommandDtoMapper.ToProto(none)).SecretsRemoved);
+
+        var empty = none with { SecretsRemoved = [] };
+        Assert.Null(SiteCommandDtoMapper.FromProto(SiteCommandDtoMapper.ToProto(empty)).SecretsRemoved);
+    }
 }
@@ -1157,6 +1157,184 @@ public class ArtifactDeploymentServiceTests : TestKit
    /// reconciliation gate can be exercised independently of the wire-level success/failure
    /// the shared <see cref="ArtifactProbeActor"/> models.
    /// </summary>
+    // ── Final review M5: a central secret-store outage is contained to the sites that need it ──
+
+    [Fact]
+    public async Task StoreOutage_FailsOnlyTheSecretReferencingSite_AndAuditsIt()
+    {
+        var secretSite = new Site("Secret Site", "sec-site") { Id = 1 };
+        var plainSite = new Site("Plain Site", "plain-site") { Id = 2 };
+        _siteRepo.GetAllSitesAsync(Arg.Any<CancellationToken>()).Returns(new List<Site> { secretSite, plainSite });
+        _siteRepo.GetDataConnectionsBySiteIdAsync(1, Arg.Any<CancellationToken>())
+            .Returns(new List<DataConnection>
+            {
+                new("GW1", "MxGateway", 1) { PrimaryConfiguration = """{"ApiKey":"secret:the-key"}""" }
+            });
+        _siteRepo.GetDataConnectionsBySiteIdAsync(2, Arg.Any<CancellationToken>())
+            .Returns(new List<DataConnection>());
+        _secretStore.GetAsync(Arg.Any<SecretName>(), Arg.Any<CancellationToken>())
+            .Returns<Task<StoredSecret?>>(_ => throw new InvalidOperationException("SQL Server unreachable"));
+
+        var recorder = new ArtifactProbeRecorder();
+        var probe = Sys.ActorOf(Props.Create(() =>
+            new SecretsAckProbeActor(recorder, cmd => cmd.Secrets == null ? (int?)null : cmd.Secrets.Count)));
+        var service = CreateServiceWithCommActor(probe);
+
+        object? audited = null;
+        _audit.When(x => x.LogAsync(
+                Arg.Any<string>(), "DeploySiteSecrets", Arg.Any<string>(), "sec-site", Arg.Any<string>(),
+                Arg.Any<object>(), Arg.Any<CancellationToken>()))
+            .Do(ci => audited = ci[5]);
+
+        // Before M5 the store exception escaped the build loop and the whole sweep threw.
+        var result = await service.DeployToAllSitesAsync("admin");
+
+        Assert.True(result.IsSuccess);
+        Assert.Equal(1, result.Value.SuccessCount);
+        Assert.Equal(1, result.Value.FailureCount);
+        var failed = Assert.Single(result.Value.SiteResults, r => r.SiteId == "sec-site");
+        Assert.False(failed.Success);
+        Assert.Contains("Central secret store could not be read", failed.ErrorMessage);
+        Assert.Contains("SQL Server unreachable", failed.ErrorMessage);
+        Assert.Contains(result.Value.SiteResults, r => r.SiteId == "plain-site" && r.Success);
+
+        // Nothing was sent to the failing site, and it keeps no stamp.
+        var sent = Assert.Single(recorder.Received);
+        Assert.Empty(sent.DataConnections ?? []);
+        Assert.Null(secretSite.ArtifactRevisionHash);
+        Assert.NotNull(plainSite.ArtifactRevisionHash);
+
+        Assert.NotNull(audited);
+        var json = JsonSerializer.Serialize(audited);
+        Assert.Contains("\"success\":false", json);
+        Assert.Contains("Central secret store could not be read", json);
+    }
+
+    [Fact]
+    public async Task ComputeCurrentHashes_StoreOutage_OmitsOnlyTheSecretReferencingSite()
+    {
+        var secretSite = new Site("Secret Site", "sec-site") { Id = 1 };
+        var plainSite = new Site("Plain Site", "plain-site") { Id = 2 };
+        _siteRepo.GetDataConnectionsBySiteIdAsync(1, Arg.Any<CancellationToken>())
+            .Returns(new List<DataConnection>
+            {
+                new("GW1", "MxGateway", 1) { PrimaryConfiguration = """{"ApiKey":"secret:the-key"}""" }
+            });
+        _siteRepo.GetDataConnectionsBySiteIdAsync(2, Arg.Any<CancellationToken>())
+            .Returns(new List<DataConnection>());
+        _secretStore.GetAsync(Arg.Any<SecretName>(), Arg.Any<CancellationToken>())
+            .Returns<Task<StoredSecret?>>(_ => throw new InvalidOperationException("SQL Server unreachable"));
+        var service = CreateService();
+
+        var hashes = await service.ComputeCurrentArtifactHashesAsync([secretSite, plainSite]);
+
+        // The secret-referencing site is left out (the evaluator reads that as Out of date);
+        // the plain site still gets its hash instead of the whole fleet check failing.
+        Assert.False(hashes.ContainsKey(1));
+        Assert.True(hashes.ContainsKey(2));
+    }
+
+    [Fact]
+    public async Task RetryForSite_StoreOutage_FailsWithoutSending()
+    {
+        _siteRepo.GetDataConnectionsBySiteIdAsync(1, Arg.Any<CancellationToken>())
+            .Returns(new List<DataConnection>
+            {
+                new("GW1", "MxGateway", 1) { PrimaryConfiguration = """{"ApiKey":"secret:the-key"}""" }
+            });
+        _secretStore.GetAsync(Arg.Any<SecretName>(), Arg.Any<CancellationToken>())
+            .Returns<Task<StoredSecret?>>(_ => throw new InvalidOperationException("SQL Server unreachable"));
+
+        var recorder = new ArtifactProbeRecorder();
+        var probe = Sys.ActorOf(Props.Create(() => new SecretsAckProbeActor(recorder, _ => 1)));
+        var service = CreateServiceWithCommActor(probe);
+
+        var result = await service.RetryForSiteAsync(1, "sec-site", "admin");
+
+        Assert.True(result.IsFailure);
+        Assert.Contains("Central secret store could not be read", result.Error);
+        Assert.Empty(recorder.Received);
+        await _audit.Received(1).LogAsync(
+            "admin", "DeploySiteSecrets", "SiteSecrets", "sec-site", Arg.Any<string>(),
+            Arg.Any<object>(), Arg.Any<CancellationToken>());
+    }
+
+    // ── Removal-only deploys are audited (the site reports the names it retired) ──
+
+    [Fact]
+    public async Task RemovalOnlyDeploy_WritesAuditRowNamingTheRemovedSecrets()
+    {
+        var site = new Site("Site One", "site-1") { Id = 1 };
+        _siteRepo.GetAllSitesAsync(Arg.Any<CancellationToken>()).Returns(new List<Site> { site });
+        // The site's last secret reference is gone: nothing ships, the site retires "old-key".
+        _siteRepo.GetDataConnectionsBySiteIdAsync(1, Arg.Any<CancellationToken>())
+            .Returns(new List<DataConnection>());
+
+        var recorder = new ArtifactProbeRecorder();
+        var probe = Sys.ActorOf(Props.Create(() => new RemovingProbeActor(recorder, new[] { "old-key" })));
+        var service = CreateServiceWithCommActor(probe);
+
+        object? audited = null;
+        _audit.When(x => x.LogAsync(
+                Arg.Any<string>(), "DeploySiteSecrets", Arg.Any<string>(), "site-1", Arg.Any<string>(),
+                Arg.Any<object>(), Arg.Any<CancellationToken>()))
+            .Do(ci => audited = ci[5]);
+
+        var result = await service.DeployToAllSitesAsync("admin");
+
+        Assert.True(result.IsSuccess);
+        var siteResult = Assert.Single(result.Value.SiteResults);
+        Assert.True(siteResult.Success);
+        Assert.Equal(["old-key"], siteResult.SecretsRemoved!);
+
+        Assert.NotNull(audited);
+        var json = JsonSerializer.Serialize(audited);
+        Assert.Contains("\"removed\":[\"old-key\"]", json);
+        Assert.Contains("\"secrets\":[]", json);
+        Assert.Contains("\"success\":true", json);
+    }
+
+    [Fact]
+    public async Task RetryForSite_RemovalOnly_WritesAuditRowNamingTheRemovedSecrets()
+    {
+        _siteRepo.GetDataConnectionsBySiteIdAsync(1, Arg.Any<CancellationToken>())
+            .Returns(new List<DataConnection>());
+
+        var recorder = new ArtifactProbeRecorder();
+        var probe = Sys.ActorOf(Props.Create(() => new RemovingProbeActor(recorder, new[] { "old-key" })));
+        var service = CreateServiceWithCommActor(probe);
+
+        object? audited = null;
+        _audit.When(x => x.LogAsync(
+                Arg.Any<string>(), "DeploySiteSecrets", Arg.Any<string>(), "site-1", Arg.Any<string>(),
+                Arg.Any<object>(), Arg.Any<CancellationToken>()))
+            .Do(ci => audited = ci[5]);
+
+        var result = await service.RetryForSiteAsync(1, "site-1", "admin");
+
+        Assert.True(result.IsSuccess);
+        Assert.NotNull(audited);
+        Assert.Contains("\"removed\":[\"old-key\"]", JsonSerializer.Serialize(audited));
+    }
+
+    /// <summary>Acks every command as applied (0 shipped secrets) and reports the given names as retired.</summary>
+    private class RemovingProbeActor : ReceiveActor
+    {
+        public RemovingProbeActor(ArtifactProbeRecorder recorder, IReadOnlyList<string> removed)
+        {
+            Receive<SiteEnvelope>(env =>
+            {
+                if (env.Message is DeployArtifactsCommand cmd)
+                {
+                    recorder.Received.Add(cmd);
+                    Sender.Tell(new ArtifactDeploymentResponse(
+                        cmd.DeploymentId, env.SiteId, true, null, DateTimeOffset.UtcNow,
+                        cmd.Secrets?.Count, removed));
+                }
+            });
+        }
+    }
+
    private class SecretsAckProbeActor : ReceiveActor
    {
        public SecretsAckProbeActor(ArtifactProbeRecorder recorder, Func<DeployArtifactsCommand, int?> secretsApplied)
@@ -100,6 +100,40 @@ public class SiteSecretsCollectorTests
        Assert.Empty(result.Missing);
    }

+    [Fact]
+    public async Task Collect_StoreThrows_ReturnsStoreError_InsteadOfThrowing()
+    {
+        // Final review M5: one site's store read failing must not abort a whole sweep.
+        var store = new FakeSecretStore(Row("a", 1)) { Fault = new InvalidOperationException("db down") };
+
+        var result = await new SiteSecretsCollector(store).CollectAsync(
+            [Dc("GW1", """{"ApiKey":"secret:a"}""")], default);
+
+        Assert.NotNull(result.StoreError);
+        Assert.Contains("'a'", result.StoreError);
+        Assert.Contains("InvalidOperationException: db down", result.StoreError);
+    }
+
+    [Fact]
+    public async Task Collect_CallerCancellation_StillPropagates()
+    {
+        using var cts = new CancellationTokenSource();
+        cts.Cancel();
+        var store = new FakeSecretStore(Row("a", 1)) { Fault = new OperationCanceledException(cts.Token) };
+
+        await Assert.ThrowsAnyAsync<OperationCanceledException>(() => new SiteSecretsCollector(store).CollectAsync(
+            [Dc("GW1", """{"ApiKey":"secret:a"}""")], cts.Token));
+    }
+
+    [Fact]
+    public async Task Collect_Success_HasNoStoreError()
+    {
+        var result = await new SiteSecretsCollector(new FakeSecretStore(Row("a", 1))).CollectAsync(
+            [Dc("GW1", """{"ApiKey":"secret:a"}""")], default);
+
+        Assert.Null(result.StoreError);
+    }
+
    /// <summary>
    /// In-memory <see cref="ISecretStore"/> stand-in. Implements only <see cref="GetAsync"/> —
    /// the sole member <see cref="SiteSecretsCollector"/> calls; every other member throws so an
@@ -112,8 +146,13 @@ public class SiteSecretsCollectorTests

        public FakeSecretStore(params StoredSecret[] rows) => _rows = rows.ToDictionary(r => r.Name);

+        /// <summary>When set, <see cref="GetAsync"/> throws this instead of reading.</summary>
+        public Exception? Fault { get; init; }
+
        public Task<StoredSecret?> GetAsync(SecretName name, CancellationToken ct) =>
-            Task.FromResult(_rows.TryGetValue(name, out var row) ? row : null);
+            Fault is not null
+                ? Task.FromException<StoredSecret?>(Fault)
+                : Task.FromResult(_rows.TryGetValue(name, out var row) ? row : null);

        public Task UpsertAsync(StoredSecret row, CancellationToken ct) => throw new NotSupportedException();

@@ -173,6 +173,35 @@ public class DeploymentManagerActorArtifactSecretsTests : TestKit, IDisposable
        Assert.Equal(0, response.SecretsApplied);
    }

+    [Fact]
+    public async Task Secrets_Dropped_ReplyNamesTheRetiredSecrets()
+    {
+        var store = new FakeSecretStore();
+        var resolver = new FakeSecretResolver();
+        var mirror = CreateMirror(store, resolver);
+        var manager = CreateDeploymentManager(mirror);
+
+        manager.Tell(ArtifactsCommand("dep-1", secrets: [Art("a", 1), Art("b", 1)]));
+        var first = ExpectMsg<ArtifactDeploymentResponse>(TimeSpan.FromSeconds(10));
+        Assert.True(first.Success);
+        Assert.Null(first.SecretsRemoved);
+
+        // The next deploy no longer references "b" (a removal-only change for that name):
+        // the reply names it so central can audit the removal.
+        manager.Tell(ArtifactsCommand("dep-2", secrets: [Art("a", 1)]));
+        var second = ExpectMsg<ArtifactDeploymentResponse>(TimeSpan.FromSeconds(10));
+        Assert.True(second.Success);
+        Assert.Equal(["b"], second.SecretsRemoved!);
+
+        // Dropping the last reference retires "a" the same way.
+        manager.Tell(ArtifactsCommand("dep-3", secrets: []));
+        var third = ExpectMsg<ArtifactDeploymentResponse>(TimeSpan.FromSeconds(10));
+        Assert.True(third.Success);
+        Assert.Equal(0, third.SecretsApplied);
+        Assert.Equal(["a"], third.SecretsRemoved!);
+        await Task.CompletedTask;
+    }
+
    [Fact]
    public async Task Secrets_MirrorFailsOne_SuccessFalse_KekIdInMessage_SecretsAppliedExcludesFailure()
    {