Fix runtime review findings

2026-04-29 10:39:49 -04:00
parent 133c83029b
commit d543679044
69 changed files with 2233 additions and 409 deletions
@@ -30,6 +30,8 @@ public sealed class GatewayOptionsTests

        Assert.Equal(30, options.Sessions.DefaultCommandTimeoutSeconds);
        Assert.Equal(64, options.Sessions.MaxSessions);
+        Assert.Equal(1800, options.Sessions.DefaultLeaseSeconds);
+        Assert.Equal(30, options.Sessions.LeaseSweepIntervalSeconds);
        Assert.False(options.Sessions.AllowMultipleEventSubscribers);

        Assert.Equal(10_000, options.Events.QueueCapacity);
@@ -45,6 +47,7 @@ public sealed class GatewayOptionsTests
        Assert.False(options.Dashboard.ShowTagValues);

        Assert.Equal(1u, options.Protocol.WorkerProtocolVersion);
+        Assert.Equal(16 * 1024 * 1024, options.Protocol.MaxGrpcMessageBytes);
    }

    [Fact]
@@ -56,22 +59,29 @@ public sealed class GatewayOptionsTests
                ["MxGateway:Authentication:Mode"] = "Disabled",
                ["MxGateway:Worker:ExecutablePath"] = @"C:\Gateway\MxGateway.Worker.exe",
                ["MxGateway:Sessions:MaxSessions"] = "12",
+                ["MxGateway:Sessions:DefaultLeaseSeconds"] = "900",
                ["MxGateway:Events:QueueCapacity"] = "256",
-                ["MxGateway:Dashboard:Enabled"] = "false"
+                ["MxGateway:Dashboard:Enabled"] = "false",
+                ["MxGateway:Protocol:MaxGrpcMessageBytes"] = "8388608"
            });

        Assert.Equal(AuthenticationMode.Disabled, options.Authentication.Mode);
        Assert.Equal(@"C:\Gateway\MxGateway.Worker.exe", options.Worker.ExecutablePath);
        Assert.Equal(12, options.Sessions.MaxSessions);
+        Assert.Equal(900, options.Sessions.DefaultLeaseSeconds);
        Assert.Equal(256, options.Events.QueueCapacity);
        Assert.False(options.Dashboard.Enabled);
+        Assert.Equal(8 * 1024 * 1024, options.Protocol.MaxGrpcMessageBytes);
    }

    [Theory]
    [InlineData("MxGateway:Worker:ExecutablePath", "worker.dll", "MxGateway:Worker:ExecutablePath must point to a .exe file.")]
    [InlineData("MxGateway:Worker:StartupProbeRetryAttempts", "0", "MxGateway:Worker:StartupProbeRetryAttempts must be greater than zero.")]
    [InlineData("MxGateway:Worker:PipeConnectAttemptTimeoutMilliseconds", "0", "MxGateway:Worker:PipeConnectAttemptTimeoutMilliseconds must be greater than zero.")]
+    [InlineData("MxGateway:Sessions:DefaultLeaseSeconds", "0", "MxGateway:Sessions:DefaultLeaseSeconds must be greater than zero.")]
+    [InlineData("MxGateway:Sessions:LeaseSweepIntervalSeconds", "0", "MxGateway:Sessions:LeaseSweepIntervalSeconds must be greater than zero.")]
    [InlineData("MxGateway:Events:QueueCapacity", "0", "MxGateway:Events:QueueCapacity must be greater than zero.")]
+    [InlineData("MxGateway:Protocol:MaxGrpcMessageBytes", "0", "MxGateway:Protocol:MaxGrpcMessageBytes must be between")]
    [InlineData("MxGateway:Authentication:PepperSecretName", "", "MxGateway:Authentication:PepperSecretName is required")]
    [InlineData("MxGateway:Dashboard:PathBase", "dashboard", "MxGateway:Dashboard:PathBase must start with '/'.")]
    public void Validation_InvalidConfiguration_FailsClearly(string key, string value, string expectedFailure)
@@ -11,9 +11,9 @@ public sealed class GatewayContractInfoTests
    }

    [Fact]
-    public void GatewayProtocolVersion_StartsAtVersionOne()
+    public void GatewayProtocolVersion_IsVersionTwo()
    {
-        Assert.Equal(1u, GatewayContractInfo.GatewayProtocolVersion);
+        Assert.Equal(2u, GatewayContractInfo.GatewayProtocolVersion);
    }

    [Fact]
@@ -15,7 +15,7 @@ public sealed class GalaxyHierarchyCacheTests
        Assert.Equal(GalaxyCacheStatus.Unknown, entry.Status);
        Assert.False(entry.HasData);
        Assert.Equal(0, entry.ObjectCount);
-        Assert.Null(entry.Reply);
+        Assert.Empty(entry.Objects);
    }

    [Fact]
@@ -0,0 +1,21 @@
+using MxGateway.Server.Dashboard;
+
+namespace MxGateway.Tests.Gateway.Dashboard;
+
+public sealed class DashboardConnectionStringDisplayTests
+{
+    [Fact]
+    public void GalaxyRepositoryConnectionString_WithSqlCredentials_OnlyKeepsNonSecretFields()
+    {
+        string display = DashboardConnectionStringDisplay.GalaxyRepositoryConnectionString(
+            "Server=localhost;Database=ZB;User ID=mxuser;Password=secret;Encrypt=True;Trust Server Certificate=False;");
+
+        Assert.Contains("Data Source=localhost", display, StringComparison.Ordinal);
+        Assert.Contains("Initial Catalog=ZB", display, StringComparison.Ordinal);
+        Assert.Contains("Encrypt=True", display, StringComparison.Ordinal);
+        Assert.DoesNotContain("User", display, StringComparison.OrdinalIgnoreCase);
+        Assert.DoesNotContain("Password", display, StringComparison.OrdinalIgnoreCase);
+        Assert.DoesNotContain("secret", display, StringComparison.OrdinalIgnoreCase);
+        Assert.DoesNotContain("mxuser", display, StringComparison.OrdinalIgnoreCase);
+    }
+}
@@ -182,17 +182,27 @@ public sealed class DashboardSnapshotServiceTests
            LastQueriedAt = DateTimeOffset.Parse("2026-04-28T11:30:00Z"),
            LastSuccessAt = DateTimeOffset.Parse("2026-04-28T11:30:00Z"),
            LastDeployTime = DateTimeOffset.Parse("2026-04-28T09:00:00Z"),
-            Hierarchy =
-            [
-                new GalaxyHierarchyRow { GobjectId = 1, TagName = "Pump_001", BrowseName = "Pump_001", CategoryId = 10, IsArea = false, TemplateChain = ["$Pump"] },
-                new GalaxyHierarchyRow { GobjectId = 2, TagName = "Pump_002", BrowseName = "Pump_002", CategoryId = 10, IsArea = false, TemplateChain = ["$Pump"] },
-                new GalaxyHierarchyRow { GobjectId = 3, TagName = "Area_A", BrowseName = "Area_A", CategoryId = 13, IsArea = true, TemplateChain = ["$Area"] },
-            ],
-            Attributes =
-            [
-                new GalaxyAttributeRow { GobjectId = 1, AttributeName = "Speed", IsHistorized = true },
-                new GalaxyAttributeRow { GobjectId = 1, AttributeName = "Status", IsAlarm = true },
-            ],
+            DashboardSummary = new DashboardGalaxySummary(
+                DashboardGalaxyStatus.Healthy,
+                LastQueriedAt: DateTimeOffset.Parse("2026-04-28T11:30:00Z"),
+                LastSuccessAt: DateTimeOffset.Parse("2026-04-28T11:30:00Z"),
+                LastDeployTime: DateTimeOffset.Parse("2026-04-28T09:00:00Z"),
+                LastError: null,
+                ObjectCount: 3,
+                AreaCount: 1,
+                AttributeCount: 2,
+                HistorizedAttributeCount: 1,
+                AlarmAttributeCount: 1,
+                TopTemplates:
+                [
+                    new DashboardGalaxyTemplateUsage("$Pump", 2),
+                    new DashboardGalaxyTemplateUsage("$Area", 1),
+                ],
+                ObjectCategories:
+                [
+                    new DashboardGalaxyCategoryCount(10, "UserDefined", 2),
+                    new DashboardGalaxyCategoryCount(13, "Area", 1),
+                ]),
            ObjectCount = 3,
            AreaCount = 1,
            AttributeCount = 2,
@@ -0,0 +1,170 @@
+using Grpc.Core;
+using Microsoft.Extensions.Logging.Abstractions;
+using MxGateway.Contracts.Proto.Galaxy;
+using MxGateway.Server.Dashboard;
+using MxGateway.Server.Galaxy;
+using MxGateway.Server.Grpc;
+
+namespace MxGateway.Tests.Gateway.Grpc;
+
+public sealed class GalaxyRepositoryGrpcServiceTests
+{
+    [Fact]
+    public async Task DiscoverHierarchy_ReturnsRequestedPageAndTotals()
+    {
+        GalaxyRepositoryGrpcService service = CreateService(CreateEntry(CreateObjects(3)));
+
+        DiscoverHierarchyReply reply = await service.DiscoverHierarchy(
+            new DiscoverHierarchyRequest
+            {
+                PageSize = 2,
+            },
+            new TestServerCallContext());
+
+        Assert.Equal(2, reply.Objects.Count);
+        Assert.Equal("Object_001", reply.Objects[0].TagName);
+        Assert.Equal("Object_002", reply.Objects[1].TagName);
+        Assert.Equal("2", reply.NextPageToken);
+        Assert.Equal(3, reply.TotalObjectCount);
+    }
+
+    [Fact]
+    public async Task DiscoverHierarchy_WithNextPageToken_ReturnsRemainingObjects()
+    {
+        GalaxyRepositoryGrpcService service = CreateService(CreateEntry(CreateObjects(3)));
+
+        DiscoverHierarchyReply reply = await service.DiscoverHierarchy(
+            new DiscoverHierarchyRequest
+            {
+                PageSize = 2,
+                PageToken = "2",
+            },
+            new TestServerCallContext());
+
+        GalaxyObject item = Assert.Single(reply.Objects);
+        Assert.Equal("Object_003", item.TagName);
+        Assert.Equal("", reply.NextPageToken);
+        Assert.Equal(3, reply.TotalObjectCount);
+    }
+
+    [Theory]
+    [InlineData("-1", 1)]
+    [InlineData("not-an-offset", 1)]
+    [InlineData("4", 1)]
+    [InlineData("", -1)]
+    public async Task DiscoverHierarchy_WithInvalidPagingArguments_ReturnsInvalidArgument(
+        string pageToken,
+        int pageSize)
+    {
+        GalaxyRepositoryGrpcService service = CreateService(CreateEntry(CreateObjects(3)));
+
+        RpcException exception = await Assert.ThrowsAsync<RpcException>(
+            async () => await service.DiscoverHierarchy(
+                new DiscoverHierarchyRequest
+                {
+                    PageSize = pageSize,
+                    PageToken = pageToken,
+                },
+                new TestServerCallContext()));
+
+        Assert.Equal(StatusCode.InvalidArgument, exception.StatusCode);
+    }
+
+    private static GalaxyRepositoryGrpcService CreateService(GalaxyHierarchyCacheEntry entry)
+    {
+        GalaxyRepositoryOptions options = new()
+        {
+            ConnectionString = "Server=localhost;Database=ZB;Integrated Security=True;Encrypt=False;",
+        };
+        return new GalaxyRepositoryGrpcService(
+            new global::MxGateway.Server.Galaxy.GalaxyRepository(options),
+            new StubGalaxyHierarchyCache(entry),
+            new GalaxyDeployNotifier(),
+            NullLogger<GalaxyRepositoryGrpcService>.Instance);
+    }
+
+    private static GalaxyHierarchyCacheEntry CreateEntry(IReadOnlyList<GalaxyObject> objects)
+    {
+        return GalaxyHierarchyCacheEntry.Empty with
+        {
+            Status = GalaxyCacheStatus.Healthy,
+            LastSuccessAt = DateTimeOffset.UtcNow,
+            Objects = objects,
+            DashboardSummary = DashboardGalaxySummary.Unknown with
+            {
+                Status = DashboardGalaxyStatus.Healthy,
+                ObjectCount = objects.Count,
+            },
+            ObjectCount = objects.Count,
+        };
+    }
+
+    private static IReadOnlyList<GalaxyObject> CreateObjects(int count)
+    {
+        return Enumerable.Range(1, count)
+            .Select(index => new GalaxyObject
+            {
+                GobjectId = index,
+                TagName = $"Object_{index:000}",
+                BrowseName = $"Object_{index:000}",
+            })
+            .ToArray();
+    }
+
+    private sealed class StubGalaxyHierarchyCache(GalaxyHierarchyCacheEntry current) : IGalaxyHierarchyCache
+    {
+        public GalaxyHierarchyCacheEntry Current { get; } = current;
+
+        public Task RefreshAsync(CancellationToken cancellationToken) => Task.CompletedTask;
+
+        public Task WaitForFirstLoadAsync(CancellationToken cancellationToken) => Task.CompletedTask;
+    }
+
+    private sealed class TestServerCallContext(CancellationToken cancellationToken = default) : ServerCallContext
+    {
+        private readonly Metadata requestHeaders = [];
+        private readonly Metadata responseTrailers = [];
+        private readonly Dictionary<object, object> userState = [];
+        private Status status;
+        private WriteOptions? writeOptions;
+
+        protected override string MethodCore => "/galaxy_repository.v1.GalaxyRepository/DiscoverHierarchy";
+
+        protected override string HostCore => "localhost";
+
+        protected override string PeerCore => "ipv4:127.0.0.1:5000";
+
+        protected override DateTime DeadlineCore => DateTime.UtcNow.AddMinutes(1);
+
+        protected override Metadata RequestHeadersCore => requestHeaders;
+
+        protected override CancellationToken CancellationTokenCore => cancellationToken;
+
+        protected override Metadata ResponseTrailersCore => responseTrailers;
+
+        protected override Status StatusCore
+        {
+            get => status;
+            set => status = value;
+        }
+
+        protected override WriteOptions? WriteOptionsCore
+        {
+            get => writeOptions;
+            set => writeOptions = value;
+        }
+
+        protected override AuthContext AuthContextCore { get; } = new(
+            string.Empty,
+            new Dictionary<string, List<AuthProperty>>(StringComparer.Ordinal));
+
+        protected override IDictionary<object, object> UserStateCore => userState;
+
+        protected override Task WriteResponseHeadersAsyncCore(Metadata responseHeaders) => Task.CompletedTask;
+
+        protected override ContextPropagationToken CreatePropagationTokenCore(ContextPropagationOptions? options)
+        {
+            throw new NotSupportedException();
+        }
+    }
+}
@@ -32,6 +32,21 @@ public sealed class SessionManagerTests
        Assert.Equal(1, metrics.GetSnapshot().SessionsOpened);
    }

+    [Fact]
+    public async Task OpenSessionAsync_SetsInitialDefaultLease()
+    {
+        ManualTimeProvider clock = new(DateTimeOffset.Parse("2026-04-29T10:00:00Z"));
+        GatewayOptions options = CreateOptions(defaultLeaseSeconds: 1800);
+        SessionManager manager = CreateManager(
+            new FakeSessionWorkerClientFactory(new FakeWorkerClient()),
+            options: options,
+            timeProvider: clock);
+
+        GatewaySession session = await manager.OpenSessionAsync(CreateOpenRequest(), "client-1", CancellationToken.None);
+
+        Assert.Equal(clock.GetUtcNow() + TimeSpan.FromMinutes(30), session.LeaseExpiresAt);
+    }
+
    [Fact]
    public async Task OpenSessionAsync_GeneratesClientCorrelationIdFromClientNameAndSessionId()
    {
@@ -77,6 +92,32 @@ public sealed class SessionManagerTests
        Assert.Equal(MxCommandKind.Ping, reply.Reply.Kind);
    }

+    [Fact]
+    public async Task InvokeAsync_WhenSessionReady_RefreshesLease()
+    {
+        GatewaySession session = new(
+            "session-lease-refresh",
+            "mxaccess",
+            "mxaccess-gateway-1-session-lease-refresh",
+            "nonce",
+            "client-1",
+            "test-session",
+            "client-correlation-1",
+            TimeSpan.FromSeconds(30),
+            TimeSpan.FromSeconds(5),
+            TimeSpan.FromSeconds(5),
+            TimeSpan.FromMinutes(30),
+            DateTimeOffset.UtcNow - TimeSpan.FromHours(1));
+        session.AttachWorkerClient(new FakeWorkerClient());
+        session.MarkReady();
+        DateTimeOffset? initialLease = session.LeaseExpiresAt;
+
+        await session.InvokeAsync(CreateCommand(MxCommandKind.Ping), CancellationToken.None);
+
+        Assert.True(session.LeaseExpiresAt > initialLease);
+        Assert.True(session.LeaseExpiresAt > DateTimeOffset.UtcNow);
+    }
+
    [Fact]
    public async Task GatewaySessionSubscribeBulkAsync_ForwardsOneBulkCommandAndReturnsResults()
    {
@@ -309,6 +350,23 @@ public sealed class SessionManagerTests
        Assert.Equal(0, activeClient.ShutdownCount);
    }

+    [Fact]
+    public async Task CloseExpiredLeasesAsync_DoesNotCloseActiveEventSubscriber()
+    {
+        FakeWorkerClient workerClient = new();
+        SessionManager manager = CreateManager(new FakeSessionWorkerClientFactory(workerClient));
+        GatewaySession session = await manager.OpenSessionAsync(CreateOpenRequest(), "client-1", CancellationToken.None);
+        DateTimeOffset now = DateTimeOffset.UtcNow;
+        session.ExtendLease(now.AddSeconds(-1));
+        using IDisposable eventSubscriber = session.AttachEventSubscriber(allowMultipleSubscribers: false);
+
+        int closedCount = await manager.CloseExpiredLeasesAsync(now, CancellationToken.None);
+
+        Assert.Equal(0, closedCount);
+        Assert.Equal(SessionState.Ready, session.State);
+        Assert.Equal(0, workerClient.ShutdownCount);
+    }
+
    [Fact]
    public async Task ShutdownAsync_ClosesAllRegisteredSessions()
    {
@@ -334,16 +392,20 @@ public sealed class SessionManagerTests
        ISessionWorkerClientFactory factory,
        ISessionRegistry? registry = null,
        GatewayMetrics? metrics = null,
-        GatewayOptions? options = null)
+        GatewayOptions? options = null,
+        TimeProvider? timeProvider = null)
    {
        return new SessionManager(
            registry ?? new SessionRegistry(),
            factory,
            Options.Create(options ?? CreateOptions()),
-            metrics ?? new GatewayMetrics());
+            metrics ?? new GatewayMetrics(),
+            timeProvider);
    }

-    private static GatewayOptions CreateOptions(int maxSessions = 64)
+    private static GatewayOptions CreateOptions(
+        int maxSessions = 64,
+        int defaultLeaseSeconds = 1800)
    {
        return new GatewayOptions
        {
@@ -351,6 +413,7 @@ public sealed class SessionManagerTests
            {
                DefaultCommandTimeoutSeconds = 30,
                MaxSessions = maxSessions,
+                DefaultLeaseSeconds = defaultLeaseSeconds,
            },
            Worker = new WorkerOptions
            {
@@ -540,4 +603,11 @@ public sealed class SessionManagerTests
            ShutdownReleased.TrySetResult();
        }
    }
+
+    private sealed class ManualTimeProvider(DateTimeOffset start) : TimeProvider
+    {
+        private DateTimeOffset _now = start;
+
+        public override DateTimeOffset GetUtcNow() => _now;
+    }
 }
@@ -137,6 +137,36 @@ public sealed class WorkerClientTests
        Assert.Equal(WorkerClientState.Faulted, client.State);
    }

+    [Fact]
+    public async Task ReadLoop_WhenClientFaults_KillsOwnedWorkerProcess()
+    {
+        await using PipePair pipePair = await PipePair.CreateAsync();
+        FakeWorkerProcess process = new();
+        await using WorkerClient client = CreateClient(
+            pipePair,
+            new WorkerClientOptions
+            {
+                EventChannelCapacity = 1,
+                HeartbeatGrace = TimeSpan.FromSeconds(30),
+                HeartbeatCheckInterval = TimeSpan.FromSeconds(30),
+            },
+            processHandle: CreateProcessHandle(process));
+        await CompleteHandshakeAsync(client, pipePair);
+
+        await pipePair.WorkerWriter.WriteAsync(
+            CreateEventEnvelope(sequence: 11, MxEventFamily.OnDataChange));
+        await pipePair.WorkerWriter.WriteAsync(
+            CreateEventEnvelope(sequence: 12, MxEventFamily.OnDataChange));
+
+        await WaitUntilAsync(
+            () => client.State == WorkerClientState.Faulted,
+            TestTimeout);
+
+        Assert.Equal(1, process.KillCount);
+        Assert.True(process.KillEntireProcessTree);
+        Assert.True(process.HasExited);
+    }
+
    [Fact]
    public async Task ReadLoop_WhenPipeDisconnects_FaultsClient()
    {
@@ -191,6 +221,20 @@ public sealed class WorkerClientTests
            $"DisposeAsync took {elapsed.TotalMilliseconds:N0}ms.");
    }

+    [Fact]
+    public async Task DisposeAsync_WhenOwnedWorkerStillRuns_KillsProcessBeforeDisposing()
+    {
+        await using PipePair pipePair = await PipePair.CreateAsync();
+        FakeWorkerProcess process = new();
+        WorkerClient client = CreateClient(pipePair, processHandle: CreateProcessHandle(process));
+
+        await client.DisposeAsync().AsTask().WaitAsync(TestTimeout);
+
+        Assert.Equal(1, process.KillCount);
+        Assert.True(process.KillEntireProcessTree);
+        Assert.True(process.Disposed);
+    }
+
    [Fact]
    public async Task ReadLoop_WhenHeartbeatArrives_UpdatesLastHeartbeatAndWorkerProcess()
    {
@@ -233,18 +277,28 @@ public sealed class WorkerClientTests
    private static WorkerClient CreateClient(
        PipePair pipePair,
        WorkerClientOptions? options = null,
-        GatewayMetrics? metrics = null)
+        GatewayMetrics? metrics = null,
+        WorkerProcessHandle? processHandle = null)
    {
        WorkerFrameProtocolOptions frameOptions = new(SessionId);
        WorkerClientConnection connection = new(
            SessionId,
            Nonce,
            pipePair.GatewayStream,
-            frameOptions);
+            frameOptions,
+            processHandle);

        return new WorkerClient(connection, options, metrics);
    }

+    private static WorkerProcessHandle CreateProcessHandle(FakeWorkerProcess process)
+    {
+        return new WorkerProcessHandle(
+            process,
+            new WorkerProcessCommandLine("MxGateway.Worker.exe", []),
+            DateTimeOffset.UtcNow);
+    }
+
    private static async Task CompleteHandshakeAsync(
        WorkerClient client,
        PipePair pipePair)
@@ -438,4 +492,40 @@ public sealed class WorkerClientTests
            await GatewayStream.DisposeAsync();
        }
    }
+
+    private sealed class FakeWorkerProcess : IWorkerProcess
+    {
+        private readonly TaskCompletionSource _exited = new(TaskCreationOptions.RunContinuationsAsynchronously);
+
+        public int Id { get; } = WorkerProcessId;
+
+        public bool HasExited { get; private set; }
+
+        public int? ExitCode { get; private set; }
+
+        public int KillCount { get; private set; }
+
+        public bool KillEntireProcessTree { get; private set; }
+
+        public bool Disposed { get; private set; }
+
+        public ValueTask WaitForExitAsync(CancellationToken cancellationToken)
+        {
+            return new ValueTask(_exited.Task.WaitAsync(cancellationToken));
+        }
+
+        public void Kill(bool entireProcessTree)
+        {
+            KillCount++;
+            KillEntireProcessTree = entireProcessTree;
+            HasExited = true;
+            ExitCode = -1;
+            _exited.TrySetResult();
+        }
+
+        public void Dispose()
+        {
+            Disposed = true;
+        }
+    }
 }