fix(secrets): Grpc hub client — per-row parse isolation (review Critical) + real-client CallInvoker tests

A single malformed row or manifest entry poisoned the whole sweep, permanently. WireToDomain.ToRows/ToManifest materialized eagerly, so one unparseable element threw out of the fetch callback, past SecretReplicationReconciler's per-batch handling, into SweepSafelyAsync's whole-sweep catch. The follower's pull set is derived from the hub's manifest, so the identical bad element arrived in the identical batch on every interval and nothing ever converged — logged as "sync failed", indistinguishable from a hub outage. Isolation now matches the Akka sibling's split exactly: WireToDomain stays a strict per-item parser and loses its two batch entry points (their absence is pinned by a reflection test), while GrpcSecretsHubClient converts each item inside its own try — the same shape as SecretReplicationActor.Materialize. A dropped item costs one warning naming the secret and a reason category, and every well-formed sibling still converges. Only parse-shaped exceptions are caught (ArgumentException, FormatException); a defect still propagates. A row whose own name is unparseable is logged under a marker rather than echoing the untrusted string, and no ciphertext, DEK or credential reaches a log line. GrpcSecretsHubClient had zero coverage — the existing fake implements the ISecretsHubReader seam directly, bypassing the header, the deadline and the conversion, which are the only things that class does. It is now driven for real through a fake CallInvoker handed to the generated client. The stale doc comment claiming the reconciler isolated the failure is replaced by one describing the mechanism that actually exists, and Endpoint gains a note that a plain-http hub sends the bearer token cleartext (doc only, no validation change). Tests 52 -> 67 in the package suite; solution green, no new warnings. Claude-Session: https://claude.ai/code/session_014WNM4vjoVksyyBraTXSZE1
2026-08-07 06:29:51 -04:00
parent c5ea3a7b59
commit 1c31e6bcb8
8 changed files with 649 additions and 73 deletions
@@ -7,6 +7,7 @@ using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
 using ZB.MOM.WW.Secrets.Abstractions;
 using ZB.MOM.WW.Secrets.Replicator.Grpc.DependencyInjection;
+using ZB.MOM.WW.Secrets.Replicator.Grpc.V1;
 using ZB.MOM.WW.Secrets.Sqlite;

 namespace ZB.MOM.WW.Secrets.Replicator.Grpc.Tests;
@@ -21,7 +22,8 @@ public sealed class GrpcSecretSyncServiceTests : IDisposable
    private readonly SqliteSecretStore _local;
    private readonly SqliteSecretStore _hubStore;
    private readonly FakeHubReader _hub;
-    private readonly RecordingLogger _logger = new();
+    private readonly RecordingLogger<GrpcSecretSyncService> _logger = new();
+    private readonly RecordingLogger<GrpcSecretsHubClient> _clientLogger = new();
    private readonly RecordingCacheInvalidator _cache = new();

    public GrpcSecretSyncServiceTests()
@@ -331,6 +333,81 @@ public sealed class GrpcSecretSyncServiceTests : IDisposable
        Assert.Equal(0, _hub.ManifestCallCount);
    }

+    [Fact]
+    public async Task A_malformed_row_never_poisons_the_batch_it_arrived_in()
+    {
+        // The failure this guards against is not "one row is lost" — it is that the pull set never
+        // changes, so the identical malformed row re-fails the identical sweep every interval and
+        // the node never converges on anything, indistinguishable in the log from a hub outage.
+        SecretRow bad = TestWire.Row("api/key", 0xBB);
+        bad.ContentType = "Kerberos";
+
+        var invoker = new FakeSecretsHubCallInvoker
+        {
+            ManifestResponse = new GetManifestResponse
+            {
+                Entries =
+                {
+                    TestWire.Entry("db/password"), TestWire.Entry("api/key"), TestWire.Entry("svc/token"),
+                },
+            },
+            SecretsResponse = new GetSecretsResponse
+            {
+                Secrets = { TestWire.Row("db/password", 0xAA), bad, TestWire.Row("svc/token", 0xCC) },
+            },
+        };
+
+        int pulled = await CreateServiceOverTheWire(invoker).SweepAsync(CancellationToken.None);
+
+        Assert.Equal(2, pulled);
+        Assert.NotNull(await _local.GetAsync(new SecretName("db/password"), CancellationToken.None));
+        Assert.NotNull(await _local.GetAsync(new SecretName("svc/token"), CancellationToken.None));
+        Assert.Null(await _local.GetAsync(new SecretName("api/key"), CancellationToken.None));
+        Assert.Single(_clientLogger.Warnings);
+    }
+
+    [Fact]
+    public async Task A_malformed_manifest_entry_never_poisons_the_manifest_it_arrived_in()
+    {
+        ManifestEntry bad = TestWire.Entry("api/key");
+        bad.UpdatedUtc = "not-a-timestamp";
+
+        var invoker = new FakeSecretsHubCallInvoker
+        {
+            ManifestResponse = new GetManifestResponse
+            {
+                Entries = { TestWire.Entry("db/password"), bad, TestWire.Entry("svc/token") },
+            },
+            SecretsResponse = new GetSecretsResponse
+            {
+                Secrets = { TestWire.Row("db/password", 0xAA), TestWire.Row("svc/token", 0xCC) },
+            },
+        };
+
+        int pulled = await CreateServiceOverTheWire(invoker).SweepAsync(CancellationToken.None);
+
+        Assert.Equal(2, pulled);
+        Assert.Equal(["db/password", "svc/token"], invoker.LastSecretsRequest!.Names);
+        Assert.Single(_clientLogger.Warnings);
+    }
+
+    // A sweep wired to the REAL hub client over a fake CallInvoker, so the parse boundary the other
+    // tests bypass (the FakeHubReader hands over domain types already) is actually exercised.
+    private GrpcSecretSyncService CreateServiceOverTheWire(FakeSecretsHubCallInvoker invoker)
+    {
+        IOptions<SecretsGrpcHubClientOptions> options = Options.Create(new SecretsGrpcHubClientOptions
+        {
+            Endpoint = "https://central:5001",
+            BearerToken = "hub-token",
+            SyncInterval = TimeSpan.FromHours(1),
+        });
+
+        var client = new GrpcSecretsHubClient(
+            new SecretsHub.SecretsHubClient(invoker), options, _clientLogger);
+
+        return new GrpcSecretSyncService(_local, client, _cache, options, _logger);
+    }
+
    [Fact]
    public void Registration_refuses_a_configuration_with_no_bearer_token()
    {
@@ -471,34 +548,4 @@ public sealed class GrpcSecretSyncServiceTests : IDisposable
        public void Invalidate(SecretName name) => Invalidated.Add(name);
    }

-    private sealed class RecordingLogger : ILogger<GrpcSecretSyncService>
-    {
-        public int InformationCount { get; private set; }
-
-        public int WarningCount { get; private set; }
-
-        public IDisposable? BeginScope<TState>(TState state) where TState : notnull => null;
-
-        public bool IsEnabled(LogLevel logLevel) => true;
-
-        public void Log<TState>(
-            LogLevel logLevel,
-            EventId eventId,
-            TState state,
-            Exception? exception,
-            Func<TState, Exception?, string> formatter)
-        {
-            switch (logLevel)
-            {
-                case LogLevel.Information:
-                    InformationCount++;
-                    break;
-                case LogLevel.Warning:
-                    WarningCount++;
-                    break;
-                default:
-                    break;
-            }
-        }
-    }
 }