dohertj2/mxaccessgw
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

code-reviews: resolve Server-059/060 + Tests-041 (fixed in 9ae6bce)

Browse Source 
Dashboard Galaxy summary now copies volatile fields fresh and memoizes only the
O(N) breakdown; added the DI registration-order wiring test and the memoization
regression/guard tests. All modules back to 0 pending.
This commit is contained in:
Joseph Doherty
2026-06-25 13:44:48 -04:00
parent c004b91164
commit 8cb8fcdf57
3 changed files with 14 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files
code-reviews/README.md
+6 -7
View File
@@ -17,8 +17,8 @@ Each module's `findings.md` is the source of truth; this file is generated from
| [Client.Rust](Client.Rust/findings.md) | Claude Code | 2026-06-18 | `88915c3` | Re-reviewed | 0 | 40 |
| [Contracts](Contracts/findings.md) | Claude Code | 2026-06-18 | `88915c3` | Re-reviewed | 0 | 25 |
| [IntegrationTests](IntegrationTests/findings.md) | Claude Code | 2026-06-18 | `88915c3` | Re-reviewed | 0 | 33 |
| [Server](Server/findings.md) | Claude Code | 2026-06-25 | `3cd7776` | Re-reviewed | 2 | 60 |
| [Tests](Tests/findings.md) | Claude Code | 2026-06-25 | `3cd7776` | Re-reviewed | 1 | 41 |
| [Server](Server/findings.md) | Claude Code | 2026-06-25 | `3cd7776` | Re-reviewed | 0 | 60 |
| [Tests](Tests/findings.md) | Claude Code | 2026-06-25 | `3cd7776` | Re-reviewed | 0 | 41 |
| [Worker](Worker/findings.md) | Claude Code | 2026-06-16 | `8df5ab3` | Re-reviewed | 0 | 28 |
| [Worker.Tests](Worker.Tests/findings.md) | Claude Code | 2026-06-18 | `88915c3` | Re-reviewed | 0 | 36 |
@@ -26,11 +26,7 @@ Each module's `findings.md` is the source of truth; this file is generated from
Findings with status `Open` or `In Progress`, ordered by severity.
| ID | Severity | Category | Location | Description |
|---|---|---|---|---|
| Server-059 | Medium | Correctness & logic bugs | `src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotService.cs:109-128` | `ResolveGalaxySummary` memoizes the projected dashboard Galaxy summary keyed **only** on `entry.Sequence` and returns the cached summary whenever the sequence is unchanged. The comment asserts "an unchanged sequence means the entry (and th… |
| Tests-041 | Medium | Testing coverage | `src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotServiceTests.cs`, `src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotService.cs:109-128` | `DashboardSnapshotService.ResolveGalaxySummary` introduced a lock-free memoization cache keyed on `GalaxyHierarchyCacheEntry.Sequence` (lines 109-128): on a matching sequence the cached `DashboardGalaxySummary` is returned without recomput… |
| Server-060 | Low | Testing coverage | `src/ZB.MOM.WW.MxGateway.Server/GatewayApplication.cs:95-99`, `src/ZB.MOM.WW.MxGateway.Tests/Gateway/GatewayApplicationTests.cs` | The per-key browse-subtree scoping depends entirely on the gateway's `AddSingleton<IGalaxyBrowseScopeProvider, GatewayBrowseScopeProvider>()` being registered **before** `AddZbGalaxyRepository(...)`, so that the library's `TryAddSingleton<… |
_No pending findings._
## Closed findings
@@ -137,6 +133,7 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`.
| Server-054 | Medium | Resolved | Design-document adherence | `docs/DesignDecisions.md` (Session Reconnect / Event Subscribers / Later Revisit Items §470-471), `CLAUDE.md` (Repository-Specific Conventions) |
| Server-056 | Medium | Resolved | Concurrency & thread safety | `src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionEventDistributor.cs:296-310,449-453,629-635` |
| Server-057 | Medium | Resolved | Correctness & logic bugs | `src/ZB.MOM.WW.MxGateway.Server/Sessions/GatewaySession.cs:976-1000` (`NormalizeOutboundCommand`), `:1085-1095` (`MapCommand` tracking), `gateway.md` (array-write ergonomics section), `clients/*/README.md` |
| Server-059 | Medium | Resolved | Correctness & logic bugs | `src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotService.cs:109-128` |
| Tests-003 | Medium | Resolved | Performance & resource management | `src/MxGateway.Tests/Security/Authentication/SqliteAuthStoreTests.cs:170-176`, `src/MxGateway.Tests/Security/Authentication/ApiKeyAdminCliRunnerTests.cs:252-258` |
| Tests-004 | Medium | Resolved | Testing coverage | `src/MxGateway.Tests/Security/Authorization/GatewayGrpcAuthorizationInterceptorTests.cs` |
| Tests-005 | Medium | Resolved | Testing coverage | `src/MxGateway.Tests/Gateway/Grpc/EventStreamServiceTests.cs:239-261`, `src/MxGateway.Tests/Gateway/Sessions/SessionManagerTests.cs` |
@@ -147,6 +144,7 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`.
| Tests-026 | Medium | Resolved | Testing coverage | `src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/EventStreamServiceTests.cs`, `src/ZB.MOM.WW.MxGateway.Server/Grpc/EventStreamService.cs:123-126` |
| Tests-027 | Medium | Resolved | Concurrency & thread safety | `src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/MxAccessGatewayServiceTests.cs:199-240`, `src/ZB.MOM.WW.MxGateway.Server/Metrics/GatewayMetrics.cs:8,73,246-251` |
| Tests-032 | Medium | Resolved | Testing coverage | `src/ZB.MOM.WW.MxGateway.Server/Alarms/GatewayAlarmMonitor.cs:435-441`, `src/ZB.MOM.WW.MxGateway.Tests/Alarms/GatewayAlarmMonitorProviderModeTests.cs`, `src/ZB.MOM.WW.MxGateway.Tests/Alarms/AlarmFailoverEndToEndTests.cs` |
| Tests-041 | Medium | Resolved | Testing coverage | `src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotServiceTests.cs`, `src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotService.cs:109-128` |
| Worker-004 | Medium | Resolved | Correctness & logic bugs | `src/MxGateway.Worker/Ipc/WorkerPipeSession.cs:565-588` |
| Worker-005 | Medium | Resolved | Error handling & resilience | `src/MxGateway.Worker/MxAccess/MxAccessStaSession.cs:205-258` (production alarm poll loop) |
| Worker-006 | Medium | Resolved | Correctness & logic bugs | `src/MxGateway.Worker/Ipc/WorkerPipeSession.cs:117-124`, `src/MxGateway.Worker/MxAccess/MxAccessStaSession.cs:386-491` |
@@ -386,6 +384,7 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`.
| Server-053 | Low | Resolved | Testing coverage | `src/ZB.MOM.WW.MxGateway.Tests/Alarms/AlarmWatchListResolverTests.cs`, `src/ZB.MOM.WW.MxGateway.Tests/Alarms/GatewayAlarmMonitorProviderModeTests.cs` |
| Server-055 | Low | Resolved | Correctness & logic bugs | `src/ZB.MOM.WW.MxGateway.Server/Sessions/GatewaySession.cs:842-851,1841-1871` |
| Server-058 | Low | Resolved | Testing coverage | `src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ConstraintEnforcerTests.cs` |
| Server-060 | Low | Resolved | Testing coverage | `src/ZB.MOM.WW.MxGateway.Server/GatewayApplication.cs:95-99`, `src/ZB.MOM.WW.MxGateway.Tests/Gateway/GatewayApplicationTests.cs` |
| Tests-007 | Low | Resolved | Code organization & conventions | `src/MxGateway.Tests/Gateway/Grpc/MxAccessGatewayServiceTests.cs:682`, `src/MxGateway.Tests/Gateway/Grpc/GalaxyRepositoryGrpcServiceTests.cs:324`, `src/MxGateway.Tests/Gateway/GatewayEndToEndFakeWorkerSmokeTests.cs:460`, `src/MxGateway.Tests/Security/Authorization/GatewayGrpcAuthorizationInterceptorTests.cs:233` |
| Tests-008 | Low | Resolved | mxaccessgw conventions | `src/MxGateway.Tests/Gateway/Sessions/WorkerAlarmRpcDispatcherTests.cs:1-9`, `src/MxGateway.Tests/Gateway/Sessions/NotWiredAlarmRpcDispatcherTests.cs:1-3`, `src/MxGateway.Tests/Gateway/Sessions/SessionManagerAlarmAutoSubscribeTests.cs:1` |
| Tests-009 | Low | Resolved | Documentation & comments | `src/MxGateway.Tests/Gateway/Sessions/SessionManagerTests.cs:36-37,99,365` |
code-reviews/Server/findings.md
+5 -5
View File
@@ -7,7 +7,7 @@
| Review date | 2026-06-25 |
| Commit reviewed | `3cd7776` |
| Status | Re-reviewed |
| Open findings | 2 |
| Open findings | 0 |
## Checklist coverage
@@ -1182,7 +1182,7 @@ Additionally, `GatewayAlarmMonitor.ApplyProviderModeChangeAsync` increments the
| Severity | Medium |
| Category | Correctness & logic bugs |
| Location | `src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotService.cs:109-128` |
| Status | Open |
| Status | Resolved |
**Description:** `ResolveGalaxySummary` memoizes the projected dashboard Galaxy summary keyed **only** on `entry.Sequence` and returns the cached summary whenever the sequence is unchanged. The comment asserts "an unchanged sequence means the entry (and therefore its summary) is unchanged," but that is false for the adopted `ZB.MOM.WW.GalaxyRepository` 0.2.0 cache: `Sequence` is bumped **only on a heavy refresh that detects a deploy change** (`GalaxyHierarchyCache.RefreshCoreAsync`, `nextSequence = previous.Sequence + 1`). Three other paths replace `_current` via `previous with { … }` **preserving the same `Sequence`** while changing fields the dashboard summary surfaces:
- The no-deploy-change tick (the common steady-state path) updates `LastQueriedAt`/`LastSuccessAt` (and clears `LastError`) every interval at the same sequence.
@@ -1193,7 +1193,7 @@ Because the memo keys on `Sequence` alone, the dashboard serves a stale summary:
**Recommendation:** Do not memoize the cheap, per-tick-volatile fields. Memoize only the O(N)-to-compute parts that genuinely change with `Sequence` — the `TopTemplates` and `ObjectCategories` lists derived from `entry.Objects` — keyed on `Sequence`, and build the `DashboardGalaxySummary` fresh each call from the current entry's `Status`, `LastQueriedAt`, `LastSuccessAt`, `LastDeployTime`, `LastError`, and the precomputed counts (all cheap copies). Add a `DashboardSnapshotServiceTests` case that swaps the cache's `Current` to a new entry with the **same** `Sequence` but a changed `Status`/`LastError`/`LastQueriedAt` and asserts the next `GetSnapshot().Galaxy` reflects the change. Correct the misleading memo comment.
**Resolution:** _(open)_
**Resolution:** Resolved in `9ae6bce` (2026-06-25). Split `DashboardGalaxySummaryProjector` into `ComputeBreakdown` (the O(N) template/category work, the only sequence-bound part) and `BuildSummary` (cheap volatile fields). `ResolveGalaxySummary` now memoizes only the breakdown by `Sequence` and rebuilds the summary from the current entry's `Status`/timestamps/`LastError`/counts on every tick, so a same-sequence outage surfaces `Unavailable`/`LastError`. Memo comment corrected; the redundant `DashboardGalaxyProjector` wrapper was removed. Regression test `DashboardSnapshotServiceTests.GetSnapshot_WhenGalaxyEntryChangesAtSameSequence_ReflectsVolatileStatusAndError` added (plus memoization-hit and invalidation guards).
### Server-060
@@ -1202,10 +1202,10 @@ Because the memo keys on `Sequence` alone, the dashboard serves a stale summary:
| Severity | Low |
| Category | Testing coverage |
| Location | `src/ZB.MOM.WW.MxGateway.Server/GatewayApplication.cs:95-99`, `src/ZB.MOM.WW.MxGateway.Tests/Gateway/GatewayApplicationTests.cs` |
| Status | Open |
| Status | Resolved |
**Description:** The per-key browse-subtree scoping depends entirely on the gateway's `AddSingleton<IGalaxyBrowseScopeProvider, GatewayBrowseScopeProvider>()` being registered **before** `AddZbGalaxyRepository(...)`, so that the library's `TryAddSingleton<IGalaxyBrowseScopeProvider, NullGalaxyBrowseScopeProvider>` (no scoping → full hierarchy) loses. This is the highest-blast-radius wiring in the adoption delta: if it ever regressed (the gateway line moved after `AddZbGalaxyRepository`, or the lib switched its default to a plain `AddSingleton`), **all per-API-key browse scoping would silently disable** with no error — a metadata-scoped key would see the entire Galaxy. The provider itself is well covered in isolation (`GatewayBrowseScopeProviderTests`) and end-to-end via manual construction (`GalaxyRepositoryHostWiringTests`), but **no test builds the gateway DI container and asserts `GetRequiredService<IGalaxyBrowseScopeProvider>()` resolves to `GatewayBrowseScopeProvider` rather than `NullGalaxyBrowseScopeProvider`** — the registration-order invariant is guarded only by the inline comment. (The Server-059 memo path is likewise untested; see that finding.)
**Recommendation:** Add a `GatewayApplicationTests` case that runs `GatewayApplication.CreateBuilder([])`, builds the service provider, and asserts `GetRequiredService<IGalaxyBrowseScopeProvider>()` is a `GatewayBrowseScopeProvider`. This pins the registration order and the lib's `TryAdd`-default contract so a future reorder or a lib-default change fails a fast unit test instead of silently widening data exposure.
**Resolution:** _(open)_
**Resolution:** Resolved in `9ae6bce` (2026-06-25). Added `GatewayApplicationTests.Build_RegistersGatewayBrowseScopeProviderOverLibraryDefault`, which builds the gateway via `GatewayApplication.Build([])` and asserts `GetRequiredService<IGalaxyBrowseScopeProvider>()` resolves to `GatewayBrowseScopeProvider`, pinning the registration-order invariant over the library's `TryAddSingleton` no-op default.
code-reviews/Tests/findings.md
+3 -3
View File
@@ -7,7 +7,7 @@
| Review date | 2026-06-25 |
| Commit reviewed | `3cd7776` |
| Status | Re-reviewed |
| Open findings | 1 |
| Open findings | 0 |
## Checklist coverage
@@ -789,7 +789,7 @@ The cancellation tests for `WorkerClient` in `WorkerClientTests` *do* exercise t
| Severity | Medium |
| Category | Testing coverage |
| Location | `src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotServiceTests.cs`, `src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotService.cs:109-128` |
| Status | Open |
| Status | Resolved |
**Description:** `DashboardSnapshotService.ResolveGalaxySummary` introduced a lock-free
memoization cache keyed on `GalaxyHierarchyCacheEntry.Sequence` (lines 109-128):
@@ -830,4 +830,4 @@ same `Sequence`. No test:
`MutableGalaxyHierarchyCache` with a settable `Current` property suffices; the
existing `StubGalaxyHierarchyCache` pattern is a single-line extension.
**Resolution:**
**Resolution:** Resolved in `9ae6bce` (2026-06-25). Added a `MutableGalaxyHierarchyCache` stub (settable `Current`) and three `DashboardSnapshotServiceTests` cases: `GetSnapshot_WhenGalaxyEntryChangesAtSameSequence_ReflectsVolatileStatusAndError` (the Server-059 regression), `GetSnapshot_WhenSequenceUnchanged_ReusesExpensiveTemplateBreakdown` (memoization hit — Objects swapped at the same `Sequence` stay frozen), and `GetSnapshot_WhenSequenceChanges_RecomputesTemplateBreakdown` (invalidation on `Sequence` change). The fix memoizes only the O(N) breakdown, so the assertions now target that rather than whole-summary identity.