Resolve Server-007..014 code-review findings

Server-007: GalaxyHierarchyProjector re-filtered the whole hierarchy per page (O(total) paging). It now memoizes the filtered list per cache-entry + filter signature so subsequent pages are an O(pageSize) slice. Server-008: WatchDeployEvents re-resolved browse subtrees and rebuilt globs per streamed event. ResolveBrowseSubtrees is hoisted out of the loop and GalaxyGlobMatcher caches compiled Regex instances per pattern. Server-009: auth-store connections used no busy timeout or WAL. A new OpenConnectionAsync applies journal_mode=WAL and a busy_timeout; all auth call sites use it. docs/Authentication.md updated. Server-010: the dashboard rendered Rotate/Revoke for revoked keys, where Rotate silently reactivates them. ApiKeysPage now shows actions only for Active keys. docs/Authentication.md updated. Server-011: WorkerAlarmRpcDispatcher converted to a primary constructor and brought in line with module conventions. Server-012: CLAUDE.md corrected to the canonical *:* scope strings. Server-013 (partly re-triaged): three named coverage gaps were already closed; the genuine gap (WorkerExecutableValidator) is now covered. Server-014: rewrote stale "alarm path not yet wired" comments in MxAccessGatewayService to describe the production WorkerAlarmRpcDispatcher. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-18 22:42:06 -04:00
parent a02faa6ade
commit fe9044115b
18 changed files with 552 additions and 139 deletions
@@ -88,6 +88,27 @@ public sealed class GalaxyFilterInputSafetyTests
        Assert.True(GalaxyGlobMatcher.IsMatch("Pump_001", "Pump_00?"));
    }

+    /// <summary>
+    ///     Regression guard for finding Server-008: <see cref="GalaxyGlobMatcher"/> caches
+    ///     the compiled regex per glob pattern. Repeated calls with the same pattern, and
+    ///     interleaved calls with different patterns, must keep returning the correct
+    ///     literal-vs-wildcard result rather than a stale cached match.
+    /// </summary>
+    [Fact]
+    public void GlobMatcher_RepeatedAndInterleavedPatterns_StayCorrect()
+    {
+        for (int i = 0; i < 5; i++)
+        {
+            Assert.True(GalaxyGlobMatcher.IsMatch("Pump_001", "Pump_*"));
+            Assert.False(GalaxyGlobMatcher.IsMatch("Valve_001", "Pump_*"));
+            Assert.True(GalaxyGlobMatcher.IsMatch("Valve_001", "Valve_00?"));
+            Assert.False(GalaxyGlobMatcher.IsMatch("Pump_001", "Valve_00?"));
+            // A glob equal to a SQL metacharacter still matches only its literal.
+            Assert.True(GalaxyGlobMatcher.IsMatch("%", "%"));
+            Assert.False(GalaxyGlobMatcher.IsMatch("anything", "%"));
+        }
+    }
+
    /// <summary>
    ///     Verifies a pathological glob does not cause catastrophic regex backtracking —
    ///     <see cref="GalaxyGlobMatcher"/> escapes every literal character and applies a
@@ -0,0 +1,136 @@
+using MxGateway.Contracts.Proto.Galaxy;
+using MxGateway.Server.Dashboard;
+using MxGateway.Server.Galaxy;
+
+namespace MxGateway.Tests.Galaxy;
+
+/// <summary>
+///     Direct coverage for <see cref="GalaxyHierarchyProjector"/> paging.
+///     <para>
+///         Regression guard for finding Server-007: the projector memoizes the filtered,
+///         ordered view list per <c>(cache entry, filter signature)</c> so paging is an
+///         O(pageSize) slice rather than an O(total) re-scan per page. These tests confirm
+///         the memo does not change paging results, does not bleed between distinct filter
+///         signatures, and is scoped to a single cache-entry instance.
+///     </para>
+/// </summary>
+public sealed class GalaxyHierarchyProjectorTests
+{
+    [Fact]
+    public void Project_PagedAcrossEntireHierarchy_ReturnsEveryObjectExactlyOnce()
+    {
+        GalaxyHierarchyCacheEntry entry = CreateEntry(CreateObjects(25));
+
+        List<string> collected = [];
+        int totalReported = -1;
+        for (int offset = 0; offset < 25; offset += 4)
+        {
+            GalaxyHierarchyQueryResult result = GalaxyHierarchyProjector.Project(
+                entry,
+                new DiscoverHierarchyRequest(),
+                browseSubtreeGlobs: null,
+                offset,
+                pageSize: 4);
+
+            totalReported = result.TotalObjectCount;
+            collected.AddRange(result.Objects.Select(obj => obj.TagName));
+        }
+
+        Assert.Equal(25, totalReported);
+        Assert.Equal(25, collected.Count);
+        Assert.Equal(collected.Count, collected.Distinct(StringComparer.Ordinal).Count());
+        Assert.Equal("Object_001", collected[0]);
+        Assert.Equal("Object_025", collected[^1]);
+    }
+
+    [Fact]
+    public void Project_DistinctFiltersOnSameEntry_DoNotShareMemoizedViewList()
+    {
+        GalaxyHierarchyCacheEntry entry = CreateEntry(CreateObjects(10));
+
+        GalaxyHierarchyQueryResult globbed = GalaxyHierarchyProjector.Project(
+            entry,
+            new DiscoverHierarchyRequest { TagNameGlob = "Object_00?" });
+        GalaxyHierarchyQueryResult unfiltered = GalaxyHierarchyProjector.Project(
+            entry,
+            new DiscoverHierarchyRequest());
+
+        // Distinct filter signatures must each get their own filtered list.
+        Assert.Equal(9, globbed.TotalObjectCount);
+        Assert.Equal(10, unfiltered.TotalObjectCount);
+    }
+
+    [Fact]
+    public void Project_SameFilterRepeated_ReturnsIdenticalTotals()
+    {
+        GalaxyHierarchyCacheEntry entry = CreateEntry(CreateObjects(12));
+
+        GalaxyHierarchyQueryResult first = GalaxyHierarchyProjector.Project(
+            entry,
+            new DiscoverHierarchyRequest(),
+            browseSubtreeGlobs: null,
+            offset: 0,
+            pageSize: 5);
+        GalaxyHierarchyQueryResult second = GalaxyHierarchyProjector.Project(
+            entry,
+            new DiscoverHierarchyRequest(),
+            browseSubtreeGlobs: null,
+            offset: 5,
+            pageSize: 5);
+
+        Assert.Equal(first.TotalObjectCount, second.TotalObjectCount);
+        Assert.Equal(first.FilterSignature, second.FilterSignature);
+        Assert.Equal(5, first.Objects.Count);
+        Assert.Equal(5, second.Objects.Count);
+        Assert.NotEqual(first.Objects[0].TagName, second.Objects[0].TagName);
+    }
+
+    [Fact]
+    public void Project_DistinctCacheEntries_ProjectAgainstTheirOwnData()
+    {
+        GalaxyHierarchyCacheEntry small = CreateEntry(CreateObjects(3));
+        GalaxyHierarchyCacheEntry large = CreateEntry(CreateObjects(40));
+
+        GalaxyHierarchyQueryResult smallResult = GalaxyHierarchyProjector.Project(
+            small,
+            new DiscoverHierarchyRequest());
+        GalaxyHierarchyQueryResult largeResult = GalaxyHierarchyProjector.Project(
+            large,
+            new DiscoverHierarchyRequest());
+
+        // Each entry instance keys its own memo; the second projection must not reuse the
+        // first entry's filtered view list.
+        Assert.Equal(3, smallResult.TotalObjectCount);
+        Assert.Equal(40, largeResult.TotalObjectCount);
+    }
+
+    private static GalaxyHierarchyCacheEntry CreateEntry(IReadOnlyList<GalaxyObject> objects)
+    {
+        return GalaxyHierarchyCacheEntry.Empty with
+        {
+            Status = GalaxyCacheStatus.Healthy,
+            Sequence = 1,
+            LastSuccessAt = DateTimeOffset.UtcNow,
+            Objects = objects,
+            Index = GalaxyHierarchyIndex.Build(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();
+    }
+}
@@ -0,0 +1,141 @@
+using System.Buffers.Binary;
+using MxGateway.Server.Configuration;
+using MxGateway.Server.Workers;
+
+namespace MxGateway.Tests.Gateway.Workers;
+
+/// <summary>
+///     Coverage for <see cref="WorkerExecutableValidator"/> PE-header architecture parsing
+///     (finding Server-013). The validator reads the DOS <c>MZ</c> stub, follows the PE
+///     header offset at <c>0x3c</c>, checks the <c>PE\0\0</c> signature, and compares the
+///     machine field against the required <see cref="WorkerArchitecture"/>.
+/// </summary>
+public sealed class WorkerExecutableValidatorTests : IDisposable
+{
+    private const ushort ImageFileMachineI386 = 0x014c;
+    private const ushort ImageFileMachineAmd64 = 0x8664;
+
+    private readonly List<string> _tempFiles = [];
+
+    [Fact]
+    public void Validate_X86ExecutableMatchingRequiredArchitecture_DoesNotThrow()
+    {
+        string path = WritePeFile(ImageFileMachineI386);
+
+        WorkerExecutableValidator.Validate(path, WorkerArchitecture.X86);
+    }
+
+    [Fact]
+    public void Validate_X64ExecutableMatchingRequiredArchitecture_DoesNotThrow()
+    {
+        string path = WritePeFile(ImageFileMachineAmd64);
+
+        WorkerExecutableValidator.Validate(path, WorkerArchitecture.X64);
+    }
+
+    [Fact]
+    public void Validate_X64ExecutableWhenX86Required_ThrowsInvalidExecutable()
+    {
+        string path = WritePeFile(ImageFileMachineAmd64);
+
+        WorkerProcessLaunchException exception = Assert.Throws<WorkerProcessLaunchException>(
+            () => WorkerExecutableValidator.Validate(path, WorkerArchitecture.X86));
+
+        Assert.Equal(WorkerProcessLaunchErrorCode.InvalidExecutable, exception.ErrorCode);
+        Assert.Contains("architecture", exception.Message, StringComparison.OrdinalIgnoreCase);
+    }
+
+    [Fact]
+    public void Validate_X86ExecutableWhenX64Required_ThrowsInvalidExecutable()
+    {
+        string path = WritePeFile(ImageFileMachineI386);
+
+        WorkerProcessLaunchException exception = Assert.Throws<WorkerProcessLaunchException>(
+            () => WorkerExecutableValidator.Validate(path, WorkerArchitecture.X64));
+
+        Assert.Equal(WorkerProcessLaunchErrorCode.InvalidExecutable, exception.ErrorCode);
+    }
+
+    [Fact]
+    public void Validate_FileWithoutMzHeader_ThrowsInvalidExecutable()
+    {
+        byte[] bytes = new byte[0x80];
+        // Leave the first two bytes as zero so the MZ signature check fails.
+        string path = WriteTempFile(bytes);
+
+        WorkerProcessLaunchException exception = Assert.Throws<WorkerProcessLaunchException>(
+            () => WorkerExecutableValidator.Validate(path, WorkerArchitecture.X86));
+
+        Assert.Equal(WorkerProcessLaunchErrorCode.InvalidExecutable, exception.ErrorCode);
+        Assert.Contains("MZ", exception.Message, StringComparison.Ordinal);
+    }
+
+    [Fact]
+    public void Validate_FileTooSmallForPeHeader_ThrowsInvalidExecutable()
+    {
+        string path = WriteTempFile([(byte)'M', (byte)'Z']);
+
+        WorkerProcessLaunchException exception = Assert.Throws<WorkerProcessLaunchException>(
+            () => WorkerExecutableValidator.Validate(path, WorkerArchitecture.X86));
+
+        Assert.Equal(WorkerProcessLaunchErrorCode.InvalidExecutable, exception.ErrorCode);
+    }
+
+    [Fact]
+    public void Validate_FileWithoutPeSignature_ThrowsInvalidExecutable()
+    {
+        // Build a valid MZ header pointing at a PE offset that holds a wrong signature.
+        byte[] bytes = new byte[0x100];
+        bytes[0] = (byte)'M';
+        bytes[1] = (byte)'Z';
+        BinaryPrimitives.WriteInt32LittleEndian(bytes.AsSpan(0x3c, sizeof(int)), 0x80);
+        // PE region left as zeros — the "PE\0\0" signature check fails.
+        string path = WriteTempFile(bytes);
+
+        WorkerProcessLaunchException exception = Assert.Throws<WorkerProcessLaunchException>(
+            () => WorkerExecutableValidator.Validate(path, WorkerArchitecture.X86));
+
+        Assert.Equal(WorkerProcessLaunchErrorCode.InvalidExecutable, exception.ErrorCode);
+        Assert.Contains("PE", exception.Message, StringComparison.Ordinal);
+    }
+
+    private string WritePeFile(ushort machine)
+    {
+        const int peHeaderOffset = 0x80;
+        byte[] bytes = new byte[peHeaderOffset + 6];
+        bytes[0] = (byte)'M';
+        bytes[1] = (byte)'Z';
+        BinaryPrimitives.WriteInt32LittleEndian(bytes.AsSpan(0x3c, sizeof(int)), peHeaderOffset);
+        bytes[peHeaderOffset] = (byte)'P';
+        bytes[peHeaderOffset + 1] = (byte)'E';
+        bytes[peHeaderOffset + 2] = 0;
+        bytes[peHeaderOffset + 3] = 0;
+        BinaryPrimitives.WriteUInt16LittleEndian(bytes.AsSpan(peHeaderOffset + 4, sizeof(ushort)), machine);
+        return WriteTempFile(bytes);
+    }
+
+    private string WriteTempFile(byte[] bytes)
+    {
+        string path = Path.Combine(Path.GetTempPath(), $"mxgw-pe-{Guid.NewGuid():N}.bin");
+        File.WriteAllBytes(path, bytes);
+        _tempFiles.Add(path);
+        return path;
+    }
+
+    public void Dispose()
+    {
+        foreach (string path in _tempFiles)
+        {
+            try
+            {
+                File.Delete(path);
+            }
+            catch (IOException)
+            {
+                // Best-effort cleanup of the temp PE fixtures.
+            }
+        }
+
+        _tempFiles.Clear();
+    }
+}
@@ -150,6 +150,32 @@ public sealed class SqliteAuthStoreTests : IDisposable
        Assert.Equal("matched active key", record.Details);
    }

+    /// <summary>
+    /// Verifies that <see cref="AuthSqliteConnectionFactory.OpenConnectionAsync"/> opens
+    /// the auth database in WAL journal mode so concurrent readers and writers degrade
+    /// gracefully instead of surfacing <c>SQLITE_BUSY</c> on the request path.
+    /// </summary>
+    [Fact]
+    public async Task OpenConnectionAsync_EnablesWalJournalModeAndBusyTimeout()
+    {
+        string databasePath = CreateTempDatabasePath();
+        await using ServiceProvider services = BuildAuthServices(databasePath);
+        AuthSqliteConnectionFactory factory = services.GetRequiredService<AuthSqliteConnectionFactory>();
+
+        await using SqliteConnection connection = await factory.OpenConnectionAsync(CancellationToken.None);
+
+        await using SqliteCommand journalModeCommand = connection.CreateCommand();
+        journalModeCommand.CommandText = "PRAGMA journal_mode;";
+        string? journalMode = (string?)await journalModeCommand.ExecuteScalarAsync(CancellationToken.None);
+
+        await using SqliteCommand busyTimeoutCommand = connection.CreateCommand();
+        busyTimeoutCommand.CommandText = "PRAGMA busy_timeout;";
+        long busyTimeout = (long)(await busyTimeoutCommand.ExecuteScalarAsync(CancellationToken.None) ?? 0L);
+
+        Assert.Equal("wal", journalMode, ignoreCase: true);
+        Assert.True(busyTimeout > 0, $"Expected a non-zero busy_timeout but found {busyTimeout}.");
+    }
+
    private static ServiceProvider BuildAuthServices(string databasePath)
    {
        IConfigurationRoot configuration = new ConfigurationBuilder()