From fe9044115bfabc7c9477d5f912d99e6a6caa968e Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 18 May 2026 22:42:06 -0400 Subject: [PATCH] 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) --- CLAUDE.md | 4 +- code-reviews/Server/findings.md | 34 ++--- docs/Authentication.md | 29 ++-- .../Components/Pages/ApiKeysPage.razor | 17 ++- .../Galaxy/GalaxyGlobMatcher.cs | 23 ++- .../Galaxy/GalaxyHierarchyProjector.cs | 83 ++++++++--- .../Grpc/GalaxyRepositoryGrpcService.cs | 7 +- .../Grpc/MxAccessGatewayService.cs | 47 +++--- .../AuthSqliteConnectionFactory.cs | 49 +++++- .../Authentication/SqliteApiKeyAdminStore.cs | 12 +- .../Authentication/SqliteApiKeyAuditStore.cs | 6 +- .../Authentication/SqliteApiKeyStore.cs | 6 +- .../Authentication/SqliteAuthStoreMigrator.cs | 3 +- .../Sessions/WorkerAlarmRpcDispatcher.cs | 47 +++--- .../Galaxy/GalaxyFilterInputSafetyTests.cs | 21 +++ .../Galaxy/GalaxyHierarchyProjectorTests.cs | 136 +++++++++++++++++ .../Workers/WorkerExecutableValidatorTests.cs | 141 ++++++++++++++++++ .../Authentication/SqliteAuthStoreTests.cs | 26 ++++ 18 files changed, 552 insertions(+), 139 deletions(-) create mode 100644 src/MxGateway.Tests/Galaxy/GalaxyHierarchyProjectorTests.cs create mode 100644 src/MxGateway.Tests/Gateway/Workers/WorkerExecutableValidatorTests.cs diff --git a/CLAUDE.md b/CLAUDE.md index 3f37e33..d1d2ff0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -32,7 +32,7 @@ dotnet test src/MxGateway.Worker.Tests/MxGateway.Worker.Tests.csproj -p:Platform dotnet run --project src/MxGateway.Server/MxGateway.Server.csproj # API-key admin CLI (same exe, "apikey" subcommand) -dotnet run --project src/MxGateway.Server/MxGateway.Server.csproj -- apikey create --display-name "dev" --scopes session,invoke,event,metadata,admin +dotnet run --project src/MxGateway.Server/MxGateway.Server.csproj -- apikey create --display-name "dev" --scopes session:open,session:close,invoke:read,invoke:write,invoke:secure,events:read,metadata:read,admin ``` Single test by name (xUnit `--filter`): @@ -114,7 +114,7 @@ External analysis sources referenced by design docs: ## Authentication -Gateway gRPC clients authenticate with an API key in metadata: `authorization: Bearer mxgw__`. Keys are stored hashed (with a peppered SHA) in a gateway-owned SQLite DB (default `C:\ProgramData\MxGateway\gateway-auth.db`). Scopes (`session`, `invoke`, `event`, `metadata`, `admin`) gate specific RPCs; missing → `Unauthenticated`, insufficient → `PermissionDenied`. The `apikey` subcommand on the server exe manages keys; see `src/MxGateway.Server/Security/Authentication/`. +Gateway gRPC clients authenticate with an API key in metadata: `authorization: Bearer mxgw__`. Keys are stored hashed (with a peppered SHA) in a gateway-owned SQLite DB (default `C:\ProgramData\MxGateway\gateway-auth.db`). Scopes (`session:open`, `session:close`, `invoke:read`, `invoke:write`, `invoke:secure`, `events:read`, `metadata:read`, `admin`) gate specific RPCs; missing → `Unauthenticated`, insufficient → `PermissionDenied`. The `apikey` subcommand on the server exe manages keys; see `src/MxGateway.Server/Security/Authentication/`. Dashboard auth uses the same verifier but exchanges the API key for an HTTP-only secure cookie at `/dashboard/login`. `Dashboard:AllowAnonymousLocalhost` bypasses cookie auth on loopback when explicitly enabled. diff --git a/code-reviews/Server/findings.md b/code-reviews/Server/findings.md index 4083d7b..50c2851 100644 --- a/code-reviews/Server/findings.md +++ b/code-reviews/Server/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-18 | | Commit reviewed | `6c64030` | | Status | Reviewed | -| Open findings | 8 | +| Open findings | 0 | ## Checklist coverage @@ -123,13 +123,13 @@ | Severity | Low | | Category | Performance & resource management | | Location | `src/MxGateway.Server/Galaxy/GalaxyHierarchyProjector.cs:55-70` | -| Status | Open | +| Status | Resolved | **Description:** `Project` always iterates the full `entry.Index.ObjectViews` collection and re-applies all filters to skip `offset` matched items before collecting a page. Paging through a large Galaxy hierarchy is therefore O(total) per page and O(total²/pageSize) end-to-end. The cache is in-memory so impact is bounded, but for large galaxies repeated `DiscoverHierarchy` pagination wastes CPU. **Recommendation:** Precompute and cache the filtered, ordered view list per `(filterSignature, sequence)` so subsequent pages are an O(pageSize) slice; the existing filter signature already keys page tokens. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-18. Confirmed against source: `Project` re-scanned and re-filtered the whole `ObjectViews` list on every page. Added a `ConditionalWeakTable>>` memo in `GalaxyHierarchyProjector`: the first projection of a given filter signature builds the filtered, ordered view list; subsequent pages take an O(pageSize) slice via index arithmetic. The memo is keyed on the immutable cache-entry instance, so when the cache publishes a new entry the stale memo becomes unreachable and is reclaimed with it — no explicit invalidation. `ResolveRoot` still runs before the memo lookup so a missing root surfaces `NotFound` consistently. Regression tests: `GalaxyHierarchyProjectorTests` (`Project_PagedAcrossEntireHierarchy_ReturnsEveryObjectExactlyOnce`, `Project_DistinctFiltersOnSameEntry_DoNotShareMemoizedViewList`, `Project_SameFilterRepeated_ReturnsIdenticalTotals`, `Project_DistinctCacheEntries_ProjectAgainstTheirOwnData`); existing `GalaxyRepositoryGrpcServiceTests` paging tests continue to pass unchanged. ### Server-008 @@ -138,13 +138,13 @@ | Severity | Low | | Category | Performance & resource management | | Location | `src/MxGateway.Server/Grpc/GalaxyRepositoryGrpcService.cs:111-134,160-189` | -| Status | Open | +| Status | Resolved | **Description:** `WatchDeployEvents` calls `ResolveBrowseSubtrees()` on every streamed event, and `MapDeployEvent` re-runs `GalaxyHierarchyProjector.Project` over the entire cached hierarchy (and `Sum`s attribute counts) for every event of every constrained subscriber. `GalaxyGlobMatcher.IsMatch` also rebuilds the glob regex on each call. With many constrained subscribers and frequent deploys this is avoidable work. **Recommendation:** Hoist `ResolveBrowseSubtrees()` out of the loop; compute scoped object/attribute counts once per deploy sequence and cache by `(sequence, browseSubtrees)`; cache compiled glob `Regex` instances in `GalaxyGlobMatcher`. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-18. Confirmed against source. Three changes: (1) `WatchDeployEvents` now resolves `ResolveBrowseSubtrees()` once before the streaming loop — the caller's identity and constraints are fixed for the stream lifetime, so per-event resolution was pure waste. (2) `GalaxyGlobMatcher` now caches compiled `Regex` instances in a `ConcurrentDictionary` keyed by glob pattern (with `RegexOptions.Compiled`), so the same handful of globs are translated once instead of on every `IsMatch` call. (3) The per-event `MapDeployEvent` re-projection is no longer a separate hot path: with finding Server-007 resolved, `GalaxyHierarchyProjector.Project` memoizes the filtered view list per `(cache entry, filter signature)`, so the scoped-count projection in `MapDeployEvent` for a constrained subscriber is O(matched-slice) after the first event of a given deploy sequence rather than a full re-scan — this subsumes the recommendation's `(sequence, browseSubtrees)` cache (the memo is keyed on the per-sequence cache-entry instance and the browse-subtree-bearing filter signature). Regression tests: `GalaxyFilterInputSafetyTests.GlobMatcher_RepeatedAndInterleavedPatterns_StayCorrect` (glob cache correctness); existing `WatchDeployEvents` and `GalaxyFilterInputSafetyTests` coverage continues to pass. ### Server-009 @@ -153,13 +153,13 @@ | Severity | Low | | Category | Error handling & resilience | | Location | `src/MxGateway.Server/Security/Authentication/AuthSqliteConnectionFactory.cs:15-32` | -| Status | Open | +| Status | Resolved | **Description:** Each auth-store operation opens a fresh `SqliteConnection` with no busy timeout, no WAL journal mode, and default journaling. `MarkKeyUsedAsync` runs on every authenticated request and `SqliteApiKeyAuditStore` appends on every denial; under concurrent load these writers can collide and surface `SQLITE_BUSY` as a hard failure on the request path. **Recommendation:** Set `Pooling`, a non-zero `DefaultTimeout`/`busy_timeout`, and enable WAL (`PRAGMA journal_mode=WAL`) once at startup so concurrent readers/writers degrade gracefully. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-18. Confirmed against source: the connection string set only `DataSource` and `Mode`. `AuthSqliteConnectionFactory.CreateConnection` now also sets `Pooling = true` and a non-zero `DefaultTimeout`. A new `OpenConnectionAsync(CancellationToken)` opens the connection and applies `PRAGMA journal_mode=WAL` and `PRAGMA busy_timeout` (5 s); WAL is a persistent database-level setting so re-applying it per connection is a cheap no-op, while `busy_timeout` is per-connection state. All nine auth-store call sites (`SqliteApiKeyAdminStore`, `SqliteApiKeyAuditStore`, `SqliteApiKeyStore`, `SqliteAuthStoreMigrator`) were switched from `CreateConnection()` + `OpenAsync()` to `OpenConnectionAsync()`. `docs/Authentication.md` updated to describe the WAL/busy-timeout behavior. Regression test: `SqliteAuthStoreTests.OpenConnectionAsync_EnablesWalJournalModeAndBusyTimeout`. ### Server-010 @@ -168,13 +168,13 @@ | Severity | Low | | Category | Security | | Location | `src/MxGateway.Server/Security/Authentication/SqliteApiKeyAdminStore.cs:91-114`, `src/MxGateway.Server/Dashboard/Components/Pages/ApiKeysPage.razor:168-172` | -| Status | Open | +| Status | Resolved | **Description:** `RotateAsync` sets `revoked_utc = NULL`, so rotating a previously revoked key silently reactivates it. This is documented intentional behavior in `docs/Authentication.md:167`, but the dashboard renders the "Rotate" button unconditionally — including for keys whose status badge says "Revoked" — so an operator can un-revoke a deliberately disabled key without an explicit warning. **Recommendation:** Either hide/disable the Rotate action for revoked keys in `ApiKeysPage.razor`, require an explicit confirmation, or have `RotateAsync` preserve `revoked_utc` and add a separate explicit "reactivate" operation. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-18. Confirmed against source: `ApiKeysPage.razor` rendered the Rotate button unconditionally while Revoke was already gated on `key.RevokedUtc is null`. Took the lowest-risk recommended option — the dashboard now renders the Rotate (and Revoke) actions only for keys whose status is `Active`; a revoked key shows a "No actions" placeholder, so an operator cannot un-revoke a deliberately disabled key as a side effect of a rotation. `RotateAsync`'s store-level behavior is unchanged (rotation by `key_id` still clears `revoked_utc`, which the CLI relies on); `docs/Authentication.md` updated to document both the store behavior and the dashboard restriction. No automated test added: the change is pure conditional Razor rendering and the test project has no bUnit component-rendering harness; the underlying `DashboardApiKeyManagementService` is already unit-tested. ### Server-011 @@ -183,13 +183,13 @@ | Severity | Low | | Category | Code organization & conventions | | Location | `src/MxGateway.Server/Sessions/WorkerAlarmRpcDispatcher.cs:1-46` | -| Status | Open | +| Status | Resolved | **Description:** `WorkerAlarmRpcDispatcher` deviates from the module's conventions: it fully-qualifies `System.Guid`, `System.ArgumentNullException`, and `System.Threading` types inline instead of relying on `using` directives, and uses an explicit constructor with `this.`-qualified field assignment while the rest of the module (e.g. `ConstraintEnforcer`, `MxAccessGatewayService`, `GalaxyRepositoryGrpcService`) uses primary constructors. `docs/style-guides/CSharpStyleGuide.md` is authoritative for gateway code. **Recommendation:** Add the needed `using` directives, drop the inline fully-qualified names, and convert to a primary constructor for consistency. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-18. Confirmed against source. Converted `WorkerAlarmRpcDispatcher` to a primary constructor with the standard `?? throw new ArgumentNullException(...)` field-initializer guard; dropped the inline `System.Guid` / `System.ArgumentNullException` qualifications (using implicit `using System;`); removed redundant `using System.Collections.Generic;` / `System.Threading` / `System.Threading.Tasks;` directives (covered by `ImplicitUsings`); replaced the two `if (... is null) throw new System.ArgumentNullException(...)` checks with `ArgumentNullException.ThrowIfNull`. The stale class-level ``/`` ("Replaces NotWiredAlarmRpcDispatcher once ... wired in", "partially wired", "returns an Unimplemented diagnostic") were corrected to describe the actual GUID-vs-`Provider!Group.Tag` handling — overlapping with Server-014. No behavior change, so no new test; existing `WorkerAlarmRpcDispatcherTests` continue to pass and the project builds warning-free under `TreatWarningsAsErrors`. ### Server-012 @@ -198,13 +198,13 @@ | Severity | Low | | Category | Documentation & comments | | Location | `CLAUDE.md` (Authentication section and `apikey create` example) | -| Status | Open | +| Status | Resolved | **Description:** CLAUDE.md describes scopes as `session`, `invoke`, `event`, `metadata`, `admin` and shows `apikey create --scopes session,invoke,event,metadata,admin`. The actual canonical scope strings (used by `GatewayScopes`, `GatewayGrpcScopeResolver`, and `docs/Authorization.md`) are `session:open`, `session:close`, `invoke:read`, `invoke:write`, `invoke:secure`, `events:read`, `metadata:read`, `admin`. A key created per the CLAUDE.md example carries scopes the resolver never matches. **Recommendation:** Update CLAUDE.md's scope list and the `apikey` example to the canonical `*:*` scope strings, per CLAUDE.md's own rule that docs change with the code. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-18. Confirmed against `GatewayScopes` (`session:open`, `session:close`, `invoke:read`, `invoke:write`, `invoke:secure`, `events:read`, `metadata:read`, `admin`). CLAUDE.md's Build/Test/Run `apikey create` example and the Authentication-section scope list were both updated to the canonical `*:*` strings. (Note: since finding Server-004 was resolved, the old example would now be actively rejected at create time rather than silently creating an unusable key, making the doc correction load-bearing.) Pure documentation change; no test. ### Server-013 @@ -213,13 +213,13 @@ | Severity | Low | | Category | Testing coverage | | Location | `src/MxGateway.Tests/Gateway/Dashboard/DashboardAuthorizationHandlerTests.cs`, `src/MxGateway.Tests/Gateway/GatewayApplicationTests.cs` | -| Status | Open | +| Status | Resolved | **Description:** `DashboardAuthorizationHandler` is unit-tested in isolation, but no test exercises the dashboard routes end-to-end to confirm the policy is actually enforced — which is why Server-001 (policy registered but never wired) went uncaught. There are also no tests for `WorkerExecutableValidator` (PE-header architecture parsing), `GalaxyGlobMatcher` (anchoring/escaping/empty-glob fail-open), or `GalaxyHierarchyProjector` pagination/page-token behavior. **Recommendation:** Add a `WebApplicationFactory` integration test that requests a dashboard page unauthenticated and asserts the redirect/401, plus unit tests for `WorkerExecutableValidator`, `GalaxyGlobMatcher`, and projector paging. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-18. Re-triaged against the current test suite: three of the four named gaps were already closed. (1) The dashboard route-level enforcement test exists — `GatewayApplicationTests.Build_WhenDashboardEnabled_ComponentRoutesRequireAuthorization` (and `..._AuthEndpointsAllowAnonymousAccess`), added when Server-001 was fixed. (2) `GalaxyGlobMatcher` anchoring/escaping/empty-glob behavior is covered by `GalaxyFilterInputSafetyTests` (`GlobMatcher_TreatsSqlMetacharactersAsLiterals`, `GlobMatcher_DoesNotTreatLikeWildcardsAsWildcards`, `GlobMatcher_WithPathologicalInput_DoesNotHang`), now extended with `GlobMatcher_RepeatedAndInterleavedPatterns_StayCorrect`. (3) Projector pagination/page-token behavior is covered end-to-end by `GalaxyRepositoryGrpcServiceTests` and now directly by the new `GalaxyHierarchyProjectorTests`. The one genuine remaining gap — `WorkerExecutableValidator` PE-header parsing — was closed with the new `WorkerExecutableValidatorTests` (7 cases: matching/mismatched x86 and x64, missing `MZ` header, file too small, missing `PE` signature), exercising the validator against synthesized minimal PE fixtures. ### Server-014 @@ -228,10 +228,10 @@ | Severity | Low | | Category | Documentation & comments | | Location | `src/MxGateway.Server/Grpc/MxAccessGatewayService.cs:162-171,191-198,206-214,229-237` | -| Status | Open | +| Status | Resolved | **Description:** The XML `` and inline comments on `AcknowledgeAlarm` and `QueryActiveAlarms` describe the alarm path as not yet wired and say `NotWiredAlarmRpcDispatcher` is the default ("Clients calling this method today receive an OK reply with a 'worker alarm path not yet wired' diagnostic", "an empty stream until PR A.2"). In fact `SessionServiceCollectionExtensions.AddGatewaySessions` registers `WorkerAlarmRpcDispatcher` as `IAlarmRpcDispatcher`, so DI always injects the production dispatcher; `NotWiredAlarmRpcDispatcher` is only the null fallback. The comments are stale and misleading. **Recommendation:** Update the `AcknowledgeAlarm`/`QueryActiveAlarms` remarks to reflect that `WorkerAlarmRpcDispatcher` is the wired default, and describe its actual GUID-vs-`Provider!Group.Tag` handling. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-18. Confirmed against source: `SessionServiceCollectionExtensions` registers `WorkerAlarmRpcDispatcher` as `IAlarmRpcDispatcher`, so the "not yet wired" / "empty stream until PR A.2" / "PR A.6/A.7 follow-up" prose in the `AcknowledgeAlarm` and `QueryActiveAlarms` `` and inline comments was stale. Rewrote both `` blocks and both inline comments to state that DI binds the production `WorkerAlarmRpcDispatcher`, that it routes over the worker pipe IPC, and that `AcknowledgeAlarm` handles a canonical-GUID reference (→ `AcknowledgeAlarmCommand`) and a `Provider!Group.Tag` reference (→ `AcknowledgeAlarmByNameCommand`), with `NotWiredAlarmRpcDispatcher` being only the null fallback. The matching stale `WorkerAlarmRpcDispatcher` class-level XML doc was corrected as part of Server-011. Pure documentation/comment change; no test. diff --git a/docs/Authentication.md b/docs/Authentication.md index e51b35a..4ad9f87 100644 --- a/docs/Authentication.md +++ b/docs/Authentication.md @@ -107,29 +107,20 @@ The gateway keeps API key state in a dedicated SQLite database. SQLite is suffic ### Connection factory -`AuthSqliteConnectionFactory` reads `GatewayOptions.Authentication.SqlitePath`, ensures the parent directory exists, and opens the connection in `ReadWriteCreate` mode so first-run installations can create the file without manual provisioning: +`AuthSqliteConnectionFactory` reads `GatewayOptions.Authentication.SqlitePath`, ensures the parent directory exists, and builds a connection string in `ReadWriteCreate` mode so first-run installations can create the file without manual provisioning. Connection pooling is enabled and the connection string carries a non-zero `DefaultTimeout`: ```csharp -public SqliteConnection CreateConnection() +SqliteConnectionStringBuilder builder = new() { - string sqlitePath = options.Value.Authentication.SqlitePath; - string? directory = Path.GetDirectoryName(sqlitePath); - - if (!string.IsNullOrWhiteSpace(directory)) - { - Directory.CreateDirectory(directory); - } - - SqliteConnectionStringBuilder builder = new() - { - DataSource = sqlitePath, - Mode = SqliteOpenMode.ReadWriteCreate - }; - - return new SqliteConnection(builder.ToString()); -} + DataSource = sqlitePath, + Mode = SqliteOpenMode.ReadWriteCreate, + Pooling = true, + DefaultTimeout = (int)BusyTimeout.TotalSeconds, +}; ``` +Every store opens its connection through `OpenConnectionAsync`, which opens the connection and then applies `PRAGMA journal_mode=WAL` and `PRAGMA busy_timeout`. WAL is a persistent database-level setting so re-applying it per connection is a cheap no-op; `busy_timeout` is per-connection state. Because `MarkKeyUsedAsync` runs on every authenticated request and `SqliteApiKeyAuditStore` appends on every denial, this lets concurrent readers and writers retry briefly instead of surfacing `SQLITE_BUSY` as a hard failure on the request path. + ### Schema `SqliteAuthSchema` declares table names and the current schema version as constants. Three tables are involved: @@ -166,6 +157,8 @@ public static ApiKeyRecord Read(SqliteDataReader reader) `SqliteApiKeyAdminStore` (`IApiKeyAdminStore`) implements administrative mutations: `CreateAsync` accepts an `ApiKeyCreateRequest`, `RevokeAsync` sets `revoked_utc` only when not already revoked, and `RotateAsync` replaces `secret_hash`, clears `last_used_utc`, and clears `revoked_utc` so a rotated key is immediately usable. +Because `RotateAsync` clears `revoked_utc`, rotating a previously revoked key reactivates it. The dashboard API Keys page therefore offers the Rotate (and Revoke) action only for keys whose status is `Active`; a revoked key shows no actions, so an operator cannot un-revoke a deliberately disabled key as a side effect of a rotation. + ### Audit trail `SqliteApiKeyAuditStore` (`IApiKeyAuditStore`) appends `ApiKeyAuditEntry` values to the `api_key_audit` table and stamps each row with a UTC timestamp inside the store rather than trusting the caller. `ListRecentAsync` returns the most recent rows ordered by `audit_id` descending and projects them into `ApiKeyAuditRecord`. Rows are kept even after the referenced key is revoked because the audit history is the durable record of administrative action; the `key_id` column is nullable to accommodate non-key-scoped events such as `init-db`. diff --git a/src/MxGateway.Server/Dashboard/Components/Pages/ApiKeysPage.razor b/src/MxGateway.Server/Dashboard/Components/Pages/ApiKeysPage.razor index ca5fab6..9a058a3 100644 --- a/src/MxGateway.Server/Dashboard/Components/Pages/ApiKeysPage.razor +++ b/src/MxGateway.Server/Dashboard/Components/Pages/ApiKeysPage.razor @@ -165,19 +165,26 @@ else {
- @if (key.RevokedUtc is null) { + @* Rotate clears revoked_utc, which would silently reactivate a + deliberately revoked key. Only offer it for active keys so a + revoked key is not un-revoked as a side effect of rotation. *@ + } + else + { + No actions + }
} diff --git a/src/MxGateway.Server/Galaxy/GalaxyGlobMatcher.cs b/src/MxGateway.Server/Galaxy/GalaxyGlobMatcher.cs index 51a0955..dc4f0b3 100644 --- a/src/MxGateway.Server/Galaxy/GalaxyGlobMatcher.cs +++ b/src/MxGateway.Server/Galaxy/GalaxyGlobMatcher.cs @@ -1,3 +1,4 @@ +using System.Collections.Concurrent; using System.Text; using System.Text.RegularExpressions; @@ -5,6 +6,14 @@ namespace MxGateway.Server.Galaxy; public static class GalaxyGlobMatcher { + /// + /// Compiled-regex cache keyed by glob pattern. IsMatch is called once per + /// object per DiscoverHierarchy/WatchDeployEvents evaluation, so the + /// same handful of glob patterns are translated repeatedly; caching avoids + /// rebuilding and recompiling the regex on every call. + /// + private static readonly ConcurrentDictionary RegexCache = new(StringComparer.Ordinal); + public static bool IsMatch(string value, string glob) { if (string.IsNullOrWhiteSpace(glob)) @@ -12,11 +21,15 @@ public static class GalaxyGlobMatcher return true; } - return Regex.IsMatch( - value ?? string.Empty, - BuildRegex(glob), - RegexOptions.CultureInvariant | RegexOptions.IgnoreCase, - TimeSpan.FromMilliseconds(100)); + return GetOrCreateRegex(glob).IsMatch(value ?? string.Empty); + } + + private static Regex GetOrCreateRegex(string glob) + { + return RegexCache.GetOrAdd(glob, static pattern => new Regex( + BuildRegex(pattern), + RegexOptions.CultureInvariant | RegexOptions.IgnoreCase | RegexOptions.Compiled, + TimeSpan.FromMilliseconds(100))); } private static string BuildRegex(string glob) diff --git a/src/MxGateway.Server/Galaxy/GalaxyHierarchyProjector.cs b/src/MxGateway.Server/Galaxy/GalaxyHierarchyProjector.cs index 3367082..8b0b7d3 100644 --- a/src/MxGateway.Server/Galaxy/GalaxyHierarchyProjector.cs +++ b/src/MxGateway.Server/Galaxy/GalaxyHierarchyProjector.cs @@ -1,3 +1,5 @@ +using System.Collections.Concurrent; +using System.Runtime.CompilerServices; using System.Security.Cryptography; using System.Text; using Grpc.Core; @@ -7,6 +9,18 @@ namespace MxGateway.Server.Galaxy; public static class GalaxyHierarchyProjector { + /// + /// Per-cache-entry memo of filtered, ordered lists + /// keyed by filter signature. Without it, paging through a large hierarchy + /// re-applies every filter and re-scans the full + /// collection on every page — O(total) per page, O(total²/pageSize) end-to-end. + /// With it, the first page builds the filtered list and each subsequent page is an + /// O(pageSize) slice. The table is keyed on the immutable cache-entry instance, so + /// when the cache publishes a new entry the stale memo becomes unreachable and is + /// reclaimed with it — no explicit invalidation needed. + /// + private static readonly ConditionalWeakTable>> FilteredViewCache = new(); + public static GalaxyHierarchyQueryResult Project( GalaxyHierarchyCacheEntry entry, DiscoverHierarchyRequest request, @@ -39,8 +53,6 @@ public static class GalaxyHierarchyProjector throw new ArgumentOutOfRangeException(nameof(pageSize), pageSize, "Page size must be greater than zero."); } - IReadOnlyList views = entry.Index.ObjectViews; - GalaxyObjectView? root = ResolveRoot(request, views); int? maxDepth = request.MaxDepth; if (maxDepth < 0) { @@ -49,30 +61,61 @@ public static class GalaxyHierarchyProjector "DiscoverHierarchy max_depth must be greater than or equal to zero when provided.")); } - List page = []; - int matchedCount = 0; + string filterSignature = ComputeFilterSignature(request, browseSubtreeGlobs); + IReadOnlyList matchedViews = GetFilteredViews( + entry, + request, + browseSubtreeGlobs, + maxDepth, + filterSignature); + bool includeAttributes = IncludeAttributes(request); - foreach (GalaxyObjectView view in views) + List page = new(Math.Min(pageSize, Math.Max(0, matchedViews.Count - offset))); + int end = (int)Math.Min((long)offset + pageSize, matchedViews.Count); + for (int index = offset; index < end; index++) { - if (!MatchesRoot(view, root, maxDepth) - || !MatchesBrowseSubtrees(view, browseSubtreeGlobs) - || !MatchesFilters(view.Object, request)) - { - continue; - } - - if (matchedCount >= offset && page.Count < pageSize) - { - page.Add(CloneObject(view.Object, includeAttributes)); - } - - matchedCount++; + page.Add(CloneObject(matchedViews[index].Object, includeAttributes)); } return new GalaxyHierarchyQueryResult( page, - matchedCount, - ComputeFilterSignature(request, browseSubtreeGlobs)); + matchedViews.Count, + filterSignature); + } + + private static IReadOnlyList GetFilteredViews( + GalaxyHierarchyCacheEntry entry, + DiscoverHierarchyRequest request, + IReadOnlyList? browseSubtreeGlobs, + int? maxDepth, + string filterSignature) + { + // ResolveRoot can throw RpcException(NotFound); run it before consulting the + // memo so a bad root surfaces consistently regardless of cache state. + IReadOnlyList views = entry.Index.ObjectViews; + GalaxyObjectView? root = ResolveRoot(request, views); + + ConcurrentDictionary> memo = + FilteredViewCache.GetValue(entry, static _ => new ConcurrentDictionary>(StringComparer.Ordinal)); + + return memo.GetOrAdd( + filterSignature, + static (_, state) => + { + List matched = []; + foreach (GalaxyObjectView view in state.Views) + { + if (MatchesRoot(view, state.Root, state.MaxDepth) + && MatchesBrowseSubtrees(view, state.BrowseSubtreeGlobs) + && MatchesFilters(view.Object, state.Request)) + { + matched.Add(view); + } + } + + return matched; + }, + (Views: views, Root: root, MaxDepth: maxDepth, BrowseSubtreeGlobs: browseSubtreeGlobs, Request: request)); } public static GalaxyObject? FindObjectForTag( diff --git a/src/MxGateway.Server/Grpc/GalaxyRepositoryGrpcService.cs b/src/MxGateway.Server/Grpc/GalaxyRepositoryGrpcService.cs index 8690409..a3fd2e2 100644 --- a/src/MxGateway.Server/Grpc/GalaxyRepositoryGrpcService.cs +++ b/src/MxGateway.Server/Grpc/GalaxyRepositoryGrpcService.cs @@ -115,6 +115,11 @@ public sealed class GalaxyRepositoryGrpcService( { DateTimeOffset? lastSeen = request.LastSeenDeployTime?.ToDateTimeOffset(); + // The caller's identity (and therefore its browse-subtree constraints) is fixed + // for the lifetime of the stream, so resolve the subtrees once rather than per + // streamed event. + IReadOnlyList browseSubtrees = ResolveBrowseSubtrees(); + await foreach (GalaxyDb.GalaxyDeployEventInfo info in notifier .SubscribeAsync(context.CancellationToken) .ConfigureAwait(false)) @@ -129,7 +134,7 @@ public sealed class GalaxyRepositoryGrpcService( } lastSeen = null; - await responseStream.WriteAsync(MapDeployEvent(info, ResolveBrowseSubtrees()), context.CancellationToken).ConfigureAwait(false); + await responseStream.WriteAsync(MapDeployEvent(info, browseSubtrees), context.CancellationToken).ConfigureAwait(false); } } diff --git a/src/MxGateway.Server/Grpc/MxAccessGatewayService.cs b/src/MxGateway.Server/Grpc/MxAccessGatewayService.cs index 87b4ad4..1fb3f18 100644 --- a/src/MxGateway.Server/Grpc/MxAccessGatewayService.cs +++ b/src/MxGateway.Server/Grpc/MxAccessGatewayService.cs @@ -161,13 +161,14 @@ public sealed class MxAccessGatewayService( /// /// - /// PR A.3 — surfaces the public AcknowledgeAlarm RPC. The gateway resolves the - /// session and returns a successful reply; the actual worker-side ack call ships - /// in PR A.2 which adds the MxAccess alarm subscription + worker command - /// handler. Clients calling this method today receive an OK reply with a - /// "worker alarm path not yet wired" diagnostic — no PERMISSION_DENIED, no - /// UNIMPLEMENTED, so the .NET / Python / Go / Java / Rust SDK call sites land - /// on a stable surface. + /// Surfaces the public AcknowledgeAlarm RPC. The gateway validates the request, + /// resolves the session, and delegates to the registered + /// . DI binds the production + /// , which routes + /// the ack through the worker pipe IPC: an alarm_full_reference that parses + /// as a canonical GUID forwards to AcknowledgeAlarmCommand; a + /// Provider!Group.Tag reference forwards to AcknowledgeAlarmByNameCommand; + /// anything else returns an InvalidRequest diagnostic. /// public override async Task AcknowledgeAlarm( AcknowledgeAlarmRequest request, @@ -189,11 +190,11 @@ public sealed class MxAccessGatewayService( // gRPC NotFound by the caller's MapException. _ = ResolveSession(request.SessionId); - // PR A.6 — delegate to the alarm dispatcher. NotWiredAlarmRpcDispatcher - // (default) returns OK + a worker-pending diagnostic. Production - // WorkerAlarmRpcDispatcher (dev-rig follow-up) routes through the - // worker IPC to AlarmClient.AlarmAckByGUID with full operator-identity - // fidelity. + // Delegate to the registered alarm dispatcher. DI binds the production + // WorkerAlarmRpcDispatcher, which routes the ack over the worker IPC by + // GUID (AcknowledgeAlarmCommand) or by Provider!Group.Tag reference + // (AcknowledgeAlarmByNameCommand). NotWiredAlarmRpcDispatcher is only the + // null fallback used when no dispatcher is registered. return await alarmRpcDispatcher.AcknowledgeAsync(request, context.CancellationToken) .ConfigureAwait(false); } @@ -205,12 +206,12 @@ public sealed class MxAccessGatewayService( /// /// - /// PR A.3 — surfaces the public QueryActiveAlarms RPC as an empty stream until - /// PR A.2 adds the worker-side QueryActiveAlarmsCommand that walks the - /// MxAccess active-alarm collection. Clients can call the RPC and iterate the - /// stream; today the stream completes immediately. Once A.2 ships, this - /// handler will translate the request into a WorkerCommand and stream the - /// resulting snapshots. + /// Surfaces the public QueryActiveAlarms RPC. The gateway validates the request, + /// resolves the session, and delegates to the registered + /// . DI binds the production + /// , which issues a + /// QueryActiveAlarmsCommand over the worker pipe IPC and streams each + /// ActiveAlarmSnapshot from the worker reply. /// public override async Task QueryActiveAlarms( QueryActiveAlarmsRequest request, @@ -226,11 +227,11 @@ public sealed class MxAccessGatewayService( } _ = ResolveSession(request.SessionId); - // PR A.7 — delegate to the alarm dispatcher. NotWiredAlarmRpcDispatcher - // (default) yields an empty stream. Production WorkerAlarmRpcDispatcher - // (dev-rig follow-up) walks the worker's IMxAccessAlarmConsumer - // SnapshotActiveAlarms output and translates each AlarmRecord into an - // ActiveAlarmSnapshot. + // Delegate to the registered alarm dispatcher. DI binds the production + // WorkerAlarmRpcDispatcher, which issues a QueryActiveAlarmsCommand over the + // worker IPC and streams each ActiveAlarmSnapshot from the worker reply. + // NotWiredAlarmRpcDispatcher is only the null fallback used when no + // dispatcher is registered. await foreach (ActiveAlarmSnapshot snapshot in alarmRpcDispatcher .QueryActiveAlarmsAsync(request, context.CancellationToken) .WithCancellation(context.CancellationToken) diff --git a/src/MxGateway.Server/Security/Authentication/AuthSqliteConnectionFactory.cs b/src/MxGateway.Server/Security/Authentication/AuthSqliteConnectionFactory.cs index 84a3d43..e0eae63 100644 --- a/src/MxGateway.Server/Security/Authentication/AuthSqliteConnectionFactory.cs +++ b/src/MxGateway.Server/Security/Authentication/AuthSqliteConnectionFactory.cs @@ -10,7 +10,17 @@ namespace MxGateway.Server.Security.Authentication; public sealed class AuthSqliteConnectionFactory(IOptions options) { /// - /// Creates and configures a SQLite connection to the auth database. + /// Busy timeout applied to every auth-store connection. SQLite retries a busy + /// database for this long before surfacing SQLITE_BUSY, so the concurrent + /// MarkKeyUsedAsync / audit-append writers degrade gracefully under load + /// instead of failing the request path. + /// + private static readonly TimeSpan BusyTimeout = TimeSpan.FromSeconds(5); + + /// + /// Creates an unopened SQLite connection to the auth database. Prefer + /// , which also applies WAL journaling and the + /// busy timeout. /// public SqliteConnection CreateConnection() { @@ -25,9 +35,44 @@ public sealed class AuthSqliteConnectionFactory(IOptions options SqliteConnectionStringBuilder builder = new() { DataSource = sqlitePath, - Mode = SqliteOpenMode.ReadWriteCreate + Mode = SqliteOpenMode.ReadWriteCreate, + Pooling = true, + DefaultTimeout = (int)BusyTimeout.TotalSeconds, }; return new SqliteConnection(builder.ToString()); } + + /// + /// Creates a SQLite connection, opens it, and configures WAL journaling and a + /// non-zero busy timeout so concurrent readers and writers degrade gracefully + /// rather than surfacing SQLITE_BUSY as a hard failure. + /// + public async Task OpenConnectionAsync(CancellationToken cancellationToken) + { + SqliteConnection connection = CreateConnection(); + try + { + await connection.OpenAsync(cancellationToken).ConfigureAwait(false); + await ConfigureConnectionAsync(connection, cancellationToken).ConfigureAwait(false); + return connection; + } + catch + { + await connection.DisposeAsync().ConfigureAwait(false); + throw; + } + } + + private static async Task ConfigureConnectionAsync( + SqliteConnection connection, + CancellationToken cancellationToken) + { + // WAL is a persistent, database-level setting; re-applying it per connection + // is cheap and a no-op once set. busy_timeout is per-connection state. + await using SqliteCommand command = connection.CreateCommand(); + command.CommandText = + $"PRAGMA journal_mode=WAL; PRAGMA busy_timeout={(int)BusyTimeout.TotalMilliseconds};"; + await command.ExecuteNonQueryAsync(cancellationToken).ConfigureAwait(false); + } } diff --git a/src/MxGateway.Server/Security/Authentication/SqliteApiKeyAdminStore.cs b/src/MxGateway.Server/Security/Authentication/SqliteApiKeyAdminStore.cs index f36ecea..21fe5c5 100644 --- a/src/MxGateway.Server/Security/Authentication/SqliteApiKeyAdminStore.cs +++ b/src/MxGateway.Server/Security/Authentication/SqliteApiKeyAdminStore.cs @@ -10,8 +10,7 @@ public sealed class SqliteApiKeyAdminStore(AuthSqliteConnectionFactory connectio /// public async Task CreateAsync(ApiKeyCreateRequest request, CancellationToken cancellationToken) { - await using SqliteConnection connection = connectionFactory.CreateConnection(); - await connection.OpenAsync(cancellationToken).ConfigureAwait(false); + await using SqliteConnection connection = await connectionFactory.OpenConnectionAsync(cancellationToken).ConfigureAwait(false); await using SqliteCommand command = connection.CreateCommand(); command.CommandText = """ @@ -44,8 +43,7 @@ public sealed class SqliteApiKeyAdminStore(AuthSqliteConnectionFactory connectio /// public async Task> ListAsync(CancellationToken cancellationToken) { - await using SqliteConnection connection = connectionFactory.CreateConnection(); - await connection.OpenAsync(cancellationToken).ConfigureAwait(false); + await using SqliteConnection connection = await connectionFactory.OpenConnectionAsync(cancellationToken).ConfigureAwait(false); await using SqliteCommand command = connection.CreateCommand(); command.CommandText = """ @@ -70,8 +68,7 @@ public sealed class SqliteApiKeyAdminStore(AuthSqliteConnectionFactory connectio /// public async Task RevokeAsync(string keyId, DateTimeOffset revokedUtc, CancellationToken cancellationToken) { - await using SqliteConnection connection = connectionFactory.CreateConnection(); - await connection.OpenAsync(cancellationToken).ConfigureAwait(false); + await using SqliteConnection connection = await connectionFactory.OpenConnectionAsync(cancellationToken).ConfigureAwait(false); await using SqliteCommand command = connection.CreateCommand(); command.CommandText = """ @@ -94,8 +91,7 @@ public sealed class SqliteApiKeyAdminStore(AuthSqliteConnectionFactory connectio DateTimeOffset rotatedUtc, CancellationToken cancellationToken) { - await using SqliteConnection connection = connectionFactory.CreateConnection(); - await connection.OpenAsync(cancellationToken).ConfigureAwait(false); + await using SqliteConnection connection = await connectionFactory.OpenConnectionAsync(cancellationToken).ConfigureAwait(false); await using SqliteCommand command = connection.CreateCommand(); command.CommandText = """ diff --git a/src/MxGateway.Server/Security/Authentication/SqliteApiKeyAuditStore.cs b/src/MxGateway.Server/Security/Authentication/SqliteApiKeyAuditStore.cs index 8e30aba..66b064a 100644 --- a/src/MxGateway.Server/Security/Authentication/SqliteApiKeyAuditStore.cs +++ b/src/MxGateway.Server/Security/Authentication/SqliteApiKeyAuditStore.cs @@ -7,8 +7,7 @@ public sealed class SqliteApiKeyAuditStore(AuthSqliteConnectionFactory connectio /// public async Task AppendAsync(ApiKeyAuditEntry entry, CancellationToken cancellationToken) { - await using SqliteConnection connection = connectionFactory.CreateConnection(); - await connection.OpenAsync(cancellationToken).ConfigureAwait(false); + await using SqliteConnection connection = await connectionFactory.OpenConnectionAsync(cancellationToken).ConfigureAwait(false); await using SqliteCommand command = connection.CreateCommand(); command.CommandText = """ @@ -32,8 +31,7 @@ public sealed class SqliteApiKeyAuditStore(AuthSqliteConnectionFactory connectio return []; } - await using SqliteConnection connection = connectionFactory.CreateConnection(); - await connection.OpenAsync(cancellationToken).ConfigureAwait(false); + await using SqliteConnection connection = await connectionFactory.OpenConnectionAsync(cancellationToken).ConfigureAwait(false); await using SqliteCommand command = connection.CreateCommand(); command.CommandText = """ diff --git a/src/MxGateway.Server/Security/Authentication/SqliteApiKeyStore.cs b/src/MxGateway.Server/Security/Authentication/SqliteApiKeyStore.cs index 102386a..bc5b14e 100644 --- a/src/MxGateway.Server/Security/Authentication/SqliteApiKeyStore.cs +++ b/src/MxGateway.Server/Security/Authentication/SqliteApiKeyStore.cs @@ -20,8 +20,7 @@ public sealed class SqliteApiKeyStore(AuthSqliteConnectionFactory connectionFact /// public async Task MarkKeyUsedAsync(string keyId, DateTimeOffset usedUtc, CancellationToken cancellationToken) { - await using SqliteConnection connection = connectionFactory.CreateConnection(); - await connection.OpenAsync(cancellationToken).ConfigureAwait(false); + await using SqliteConnection connection = await connectionFactory.OpenConnectionAsync(cancellationToken).ConfigureAwait(false); await using SqliteCommand command = connection.CreateCommand(); command.CommandText = """ @@ -40,8 +39,7 @@ public sealed class SqliteApiKeyStore(AuthSqliteConnectionFactory connectionFact bool requireActive, CancellationToken cancellationToken) { - await using SqliteConnection connection = connectionFactory.CreateConnection(); - await connection.OpenAsync(cancellationToken).ConfigureAwait(false); + await using SqliteConnection connection = await connectionFactory.OpenConnectionAsync(cancellationToken).ConfigureAwait(false); await using SqliteCommand command = connection.CreateCommand(); command.CommandText = requireActive diff --git a/src/MxGateway.Server/Security/Authentication/SqliteAuthStoreMigrator.cs b/src/MxGateway.Server/Security/Authentication/SqliteAuthStoreMigrator.cs index 36c0637..4d66399 100644 --- a/src/MxGateway.Server/Security/Authentication/SqliteAuthStoreMigrator.cs +++ b/src/MxGateway.Server/Security/Authentication/SqliteAuthStoreMigrator.cs @@ -8,8 +8,7 @@ public sealed class SqliteAuthStoreMigrator(AuthSqliteConnectionFactory connecti /// Cancellation token. public async Task MigrateAsync(CancellationToken cancellationToken) { - await using SqliteConnection connection = connectionFactory.CreateConnection(); - await connection.OpenAsync(cancellationToken).ConfigureAwait(false); + await using SqliteConnection connection = await connectionFactory.OpenConnectionAsync(cancellationToken).ConfigureAwait(false); await using SqliteTransaction transaction = (SqliteTransaction)await connection.BeginTransactionAsync(cancellationToken).ConfigureAwait(false); diff --git a/src/MxGateway.Server/Sessions/WorkerAlarmRpcDispatcher.cs b/src/MxGateway.Server/Sessions/WorkerAlarmRpcDispatcher.cs index 81b1561..e071bb9 100644 --- a/src/MxGateway.Server/Sessions/WorkerAlarmRpcDispatcher.cs +++ b/src/MxGateway.Server/Sessions/WorkerAlarmRpcDispatcher.cs @@ -1,7 +1,4 @@ -using System.Collections.Generic; using System.Runtime.CompilerServices; -using System.Threading; -using System.Threading.Tasks; using Google.Protobuf.WellKnownTypes; using MxGateway.Contracts.Proto; using MxGateway.Server.Grpc; @@ -11,39 +8,33 @@ namespace MxGateway.Server.Sessions; /// /// Production that routes the public /// AcknowledgeAlarm + QueryActiveAlarms RPCs through the -/// worker pipe IPC. Replaces -/// once the worker AlarmCommandHandler is wired in. +/// worker pipe IPC. DI binds this dispatcher; +/// is only the null fallback used when no dispatcher is registered. /// /// /// -/// QueryActiveAlarms is fully wired: issues a +/// QueryActiveAlarms issues a /// over the pipe and yields /// each from the /// . /// /// -/// AcknowledgeAlarm is partially wired: the public RPC's -/// is a -/// Provider!Group.Tag string, but the worker's wnwrap consumer -/// acks by GUID. When the supplied reference parses as a GUID -/// directly, the dispatcher forwards it as-is. Otherwise it -/// returns an Unimplemented diagnostic. Resolving -/// reference→GUID requires an additional worker IPC command -/// (e.g. AlarmAckByName wrapping -/// wwAlarmConsumerClass.AlarmAckByName) and is tracked as -/// a follow-up. +/// AcknowledgeAlarm accepts either form of +/// : a canonical +/// GUID forwards as an ; a +/// Provider!Group.Tag reference is parsed by +/// and forwarded as an +/// . Any other reference +/// returns an InvalidRequest diagnostic. /// /// -public sealed class WorkerAlarmRpcDispatcher : IAlarmRpcDispatcher +public sealed class WorkerAlarmRpcDispatcher( + ISessionRegistry sessionRegistry, + TimeProvider? timeProvider = null) : IAlarmRpcDispatcher { - private readonly ISessionRegistry sessionRegistry; - private readonly TimeProvider timeProvider; - - public WorkerAlarmRpcDispatcher(ISessionRegistry sessionRegistry, TimeProvider? timeProvider = null) - { - this.sessionRegistry = sessionRegistry ?? throw new System.ArgumentNullException(nameof(sessionRegistry)); - this.timeProvider = timeProvider ?? TimeProvider.System; - } + private readonly ISessionRegistry sessionRegistry = sessionRegistry + ?? throw new ArgumentNullException(nameof(sessionRegistry)); + private readonly TimeProvider timeProvider = timeProvider ?? TimeProvider.System; /// /// Parse a full alarm reference of the form Provider!Group.Tag @@ -83,7 +74,7 @@ public sealed class WorkerAlarmRpcDispatcher : IAlarmRpcDispatcher AcknowledgeAlarmRequest request, CancellationToken cancellationToken) { - if (request is null) throw new System.ArgumentNullException(nameof(request)); + ArgumentNullException.ThrowIfNull(request); if (!sessionRegistry.TryGet(request.SessionId, out GatewaySession session)) { @@ -98,7 +89,7 @@ public sealed class WorkerAlarmRpcDispatcher : IAlarmRpcDispatcher } WorkerCommand workerCommand; - if (System.Guid.TryParse(request.AlarmFullReference, out System.Guid guid)) + if (Guid.TryParse(request.AlarmFullReference, out Guid guid)) { workerCommand = new WorkerCommand { @@ -193,7 +184,7 @@ public sealed class WorkerAlarmRpcDispatcher : IAlarmRpcDispatcher QueryActiveAlarmsRequest request, [EnumeratorCancellation] CancellationToken cancellationToken) { - if (request is null) throw new System.ArgumentNullException(nameof(request)); + ArgumentNullException.ThrowIfNull(request); if (!sessionRegistry.TryGet(request.SessionId, out GatewaySession session)) { diff --git a/src/MxGateway.Tests/Galaxy/GalaxyFilterInputSafetyTests.cs b/src/MxGateway.Tests/Galaxy/GalaxyFilterInputSafetyTests.cs index 676b91f..eb1a2b5 100644 --- a/src/MxGateway.Tests/Galaxy/GalaxyFilterInputSafetyTests.cs +++ b/src/MxGateway.Tests/Galaxy/GalaxyFilterInputSafetyTests.cs @@ -88,6 +88,27 @@ public sealed class GalaxyFilterInputSafetyTests Assert.True(GalaxyGlobMatcher.IsMatch("Pump_001", "Pump_00?")); } + /// + /// Regression guard for finding Server-008: 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. + /// + [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", "%")); + } + } + /// /// Verifies a pathological glob does not cause catastrophic regex backtracking — /// escapes every literal character and applies a diff --git a/src/MxGateway.Tests/Galaxy/GalaxyHierarchyProjectorTests.cs b/src/MxGateway.Tests/Galaxy/GalaxyHierarchyProjectorTests.cs new file mode 100644 index 0000000..938ba3d --- /dev/null +++ b/src/MxGateway.Tests/Galaxy/GalaxyHierarchyProjectorTests.cs @@ -0,0 +1,136 @@ +using MxGateway.Contracts.Proto.Galaxy; +using MxGateway.Server.Dashboard; +using MxGateway.Server.Galaxy; + +namespace MxGateway.Tests.Galaxy; + +/// +/// Direct coverage for paging. +/// +/// Regression guard for finding Server-007: the projector memoizes the filtered, +/// ordered view list per (cache entry, filter signature) 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. +/// +/// +public sealed class GalaxyHierarchyProjectorTests +{ + [Fact] + public void Project_PagedAcrossEntireHierarchy_ReturnsEveryObjectExactlyOnce() + { + GalaxyHierarchyCacheEntry entry = CreateEntry(CreateObjects(25)); + + List 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 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 CreateObjects(int count) + { + return Enumerable.Range(1, count) + .Select(index => new GalaxyObject + { + GobjectId = index, + TagName = $"Object_{index:000}", + BrowseName = $"Object_{index:000}", + }) + .ToArray(); + } +} diff --git a/src/MxGateway.Tests/Gateway/Workers/WorkerExecutableValidatorTests.cs b/src/MxGateway.Tests/Gateway/Workers/WorkerExecutableValidatorTests.cs new file mode 100644 index 0000000..ae3c993 --- /dev/null +++ b/src/MxGateway.Tests/Gateway/Workers/WorkerExecutableValidatorTests.cs @@ -0,0 +1,141 @@ +using System.Buffers.Binary; +using MxGateway.Server.Configuration; +using MxGateway.Server.Workers; + +namespace MxGateway.Tests.Gateway.Workers; + +/// +/// Coverage for PE-header architecture parsing +/// (finding Server-013). The validator reads the DOS MZ stub, follows the PE +/// header offset at 0x3c, checks the PE\0\0 signature, and compares the +/// machine field against the required . +/// +public sealed class WorkerExecutableValidatorTests : IDisposable +{ + private const ushort ImageFileMachineI386 = 0x014c; + private const ushort ImageFileMachineAmd64 = 0x8664; + + private readonly List _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( + () => 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( + () => 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( + () => 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( + () => 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( + () => 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(); + } +} diff --git a/src/MxGateway.Tests/Security/Authentication/SqliteAuthStoreTests.cs b/src/MxGateway.Tests/Security/Authentication/SqliteAuthStoreTests.cs index 752f1ab..ce5c52b 100644 --- a/src/MxGateway.Tests/Security/Authentication/SqliteAuthStoreTests.cs +++ b/src/MxGateway.Tests/Security/Authentication/SqliteAuthStoreTests.cs @@ -150,6 +150,32 @@ public sealed class SqliteAuthStoreTests : IDisposable Assert.Equal("matched active key", record.Details); } + /// + /// Verifies that opens + /// the auth database in WAL journal mode so concurrent readers and writers degrade + /// gracefully instead of surfacing SQLITE_BUSY on the request path. + /// + [Fact] + public async Task OpenConnectionAsync_EnablesWalJournalModeAndBusyTimeout() + { + string databasePath = CreateTempDatabasePath(); + await using ServiceProvider services = BuildAuthServices(databasePath); + AuthSqliteConnectionFactory factory = services.GetRequiredService(); + + 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()