From 362ba8d75fc14f41cf24b69cb24460f464856911 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 9 Nov 2026 14:52:00 -0500 Subject: [PATCH] fix(galaxyrepository): P3 review fixes 003-008 - cycle re-rooting, scaled stale threshold, notifier ordering, DI tests, Pkcs 9.0.21 pin, snapshot docs; bump 0.2.2 (scadaproj#47) Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV --- ZB.MOM.WW.GalaxyRepository/CLAUDE.md | 18 +- .../Directory.Build.props | 2 +- .../Directory.Packages.props | 5 + .../GalaxyDeployNotifier.cs | 9 + .../GalaxyHierarchyCache.cs | 38 +++- .../GalaxyHierarchyIndex.cs | 105 +++++++++ .../GalaxyHierarchySnapshotStore.cs | 7 + .../GalaxyRepositoryOptions.cs | 5 +- .../ZB.MOM.WW.GalaxyRepository.csproj | 3 +- .../ReviewP3Tests.cs | 200 ++++++++++++++++++ code-reviews/GalaxyRepository/findings.md | 26 +-- code-reviews/README.md | 16 +- 12 files changed, 396 insertions(+), 38 deletions(-) create mode 100644 ZB.MOM.WW.GalaxyRepository/tests/ZB.MOM.WW.GalaxyRepository.Tests/ReviewP3Tests.cs diff --git a/ZB.MOM.WW.GalaxyRepository/CLAUDE.md b/ZB.MOM.WW.GalaxyRepository/CLAUDE.md index ecaf6c3..4ff9bc8 100644 --- a/ZB.MOM.WW.GalaxyRepository/CLAUDE.md +++ b/ZB.MOM.WW.GalaxyRepository/CLAUDE.md @@ -2,6 +2,17 @@ Shared Galaxy object-hierarchy SQL browse + `galaxy_repository.v1` gRPC service for the **ZB.MOM.WW SCADA family** (consumed by HistorianGateway and mxaccessgw). Build/test from this directory: `dotnet test`. +## 0.2.2 (2026-11-09, UNPUBLISHED) + +Version bumped in `Directory.Build.props`, not packed or pushed. P3 review fixes (scadaproj#47): +- 003: parent cycles are broken at the lowest gobject id so their members stay browsable. +- 004: Stale threshold is `max(5 min, 3 x DashboardRefreshIntervalSeconds)` (new optional `IOptions` ctor + parameter on `GalaxyHierarchyCache`); the projected-Stale entry is memoized per source entry. +- 005: `WatchDeployEvents` subscribers never see a non-increasing `Sequence`. +- 006: container-building tests for `AddZbGalaxyRepository`. +- 007: `System.Security.Cryptography.Pkcs` pinned at 9.0.21 (clears NU1902/NU1903 for consumers). +- 008: empty `SnapshotCachePath` disables persistence (documented + logged once). + ## 0.2.1 (2026-11-09, UNPUBLISHED) Version bumped in `Directory.Build.props`, not packed or pushed; consumers stay on `0.2.0`. @@ -34,7 +45,6 @@ the Gitea NuGet feed** `gitea.dohertylan.com/api/packages/dohertj2/nuget`). The [`docs/plans/2026-06-23-historian-gateway-design.md`](docs/plans/2026-06-23-historian-gateway-design.md) (§10, component 1). **Consumed by HistorianGateway** as a `PackageReference` from that Gitea feed, pinned at `0.2.0` (originally a cross-repo `ProjectReference` to this scadaproj tree; switched to the feed package 2026-06-24; bumped `0.1.0` → `0.2.0` 2026-06-25). -**mxaccessgw adoption is a tracked follow-on** — once adopted, mxaccessgw's inline Galaxy browse code is replaced -by the shared lib (the `galaxy_repository.v1` wire contract is already identical, so OtOpcUa and ScadaBridge -clients are unaffected). Build/test from `ZB.MOM.WW.GalaxyRepository/`: `dotnet test`. -Consumer matrix: HistorianGateway (initial); mxaccessgw (follow-on adoption). +**mxaccessgw has adopted it** (its Server calls `AddZbGalaxyRepository` / `MapZbGalaxyRepository`; the +`galaxy_repository.v1` wire contract is identical, so OtOpcUa and ScadaBridge clients are unaffected). Build/test from `ZB.MOM.WW.GalaxyRepository/`: `dotnet test`. +Consumer matrix: HistorianGateway (initial); mxaccessgw (adopted, 0.2.0). diff --git a/ZB.MOM.WW.GalaxyRepository/Directory.Build.props b/ZB.MOM.WW.GalaxyRepository/Directory.Build.props index 48488ee..d0e6e3b 100644 --- a/ZB.MOM.WW.GalaxyRepository/Directory.Build.props +++ b/ZB.MOM.WW.GalaxyRepository/Directory.Build.props @@ -4,7 +4,7 @@ enable enable latest - 0.2.1 + 0.2.2 true true diff --git a/ZB.MOM.WW.GalaxyRepository/Directory.Packages.props b/ZB.MOM.WW.GalaxyRepository/Directory.Packages.props index 5250626..300346b 100644 --- a/ZB.MOM.WW.GalaxyRepository/Directory.Packages.props +++ b/ZB.MOM.WW.GalaxyRepository/Directory.Packages.props @@ -7,6 +7,11 @@ + + diff --git a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyDeployNotifier.cs b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyDeployNotifier.cs index 9658d81..5e88d03 100644 --- a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyDeployNotifier.cs +++ b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyDeployNotifier.cs @@ -60,12 +60,21 @@ public sealed class GalaxyDeployNotifier : IGalaxyDeployNotifier channel.Writer.TryWrite(bootstrap); } + long lastSequence = long.MinValue; try { while (await channel.Reader.WaitToReadAsync(cancellationToken).ConfigureAwait(false)) { while (channel.Reader.TryRead(out GalaxyDeployEventInfo? next)) { + // The bootstrap write can race a concurrent Publish and land behind a + // newer event; never yield an event that is not newer than the last one. + if (next.Sequence <= lastSequence) + { + continue; + } + + lastSequence = next.Sequence; yield return next; } } diff --git a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyCache.cs b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyCache.cs index 51ebd0c..a31b477 100644 --- a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyCache.cs +++ b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyCache.cs @@ -16,17 +16,22 @@ namespace ZB.MOM.WW.GalaxyRepository; /// public sealed class GalaxyHierarchyCache : IGalaxyHierarchyCache, IDisposable { - private static readonly TimeSpan StaleThreshold = TimeSpan.FromMinutes(5); + private static readonly TimeSpan MinimumStaleThreshold = TimeSpan.FromMinutes(5); private readonly IGalaxyRepository _repository; private readonly IGalaxyDeployNotifier _notifier; private readonly IGalaxyHierarchySnapshotStore? _snapshotStore; private readonly TimeProvider _timeProvider; + private readonly TimeSpan _staleThreshold; private readonly ILogger? _logger; private readonly TaskCompletionSource _firstLoad = new(TaskCreationOptions.RunContinuationsAsynchronously); private readonly SemaphoreSlim _refreshGate = new(1, 1); private GalaxyHierarchyCacheEntry _current = GalaxyHierarchyCacheEntry.Empty; private bool _restoreAttempted; + private ProjectedStale? _projectedStale; + + /// Memoized status-only projection so repeated reads return the same instance (keeps projector memos warm). + private sealed record ProjectedStale(GalaxyHierarchyCacheEntry Source, GalaxyHierarchyCacheEntry Projected); /// Initializes a new instance of the class. /// Galaxy Repository client for SQL queries. @@ -37,13 +42,21 @@ public sealed class GalaxyHierarchyCache : IGalaxyHierarchyCache, IDisposable /// Optional on-disk snapshot store. When supplied, the cache persists each /// successful refresh and restores the last snapshot on first load. /// + /// + /// Optional options; the staleness threshold is max(5 min, 3 x DashboardRefreshIntervalSeconds) + /// so a healthy cache on a slow refresh interval is not reported Stale. + /// public GalaxyHierarchyCache( IGalaxyRepository repository, IGalaxyDeployNotifier notifier, TimeProvider? timeProvider = null, ILogger? logger = null, - IGalaxyHierarchySnapshotStore? snapshotStore = null) + IGalaxyHierarchySnapshotStore? snapshotStore = null, + Microsoft.Extensions.Options.IOptions? options = null) { + int refreshSeconds = options?.Value.DashboardRefreshIntervalSeconds ?? 0; + TimeSpan derived = TimeSpan.FromSeconds(Math.Max(0, refreshSeconds) * 3.0); + _staleThreshold = derived > MinimumStaleThreshold ? derived : MinimumStaleThreshold; _repository = repository; _notifier = notifier; _timeProvider = timeProvider ?? TimeProvider.System; @@ -58,9 +71,22 @@ public sealed class GalaxyHierarchyCache : IGalaxyHierarchyCache, IDisposable { GalaxyHierarchyCacheEntry snapshot = Volatile.Read(ref _current); GalaxyCacheStatus projected = ProjectStatus(snapshot); - return projected == snapshot.Status - ? snapshot - : snapshot with { Status = projected }; + if (projected == snapshot.Status) + { + return snapshot; + } + + // Reuse the projected record while the source entry is unchanged: the projectors + // memoize per entry instance, so a fresh record per read defeats the memo. + ProjectedStale? memo = Volatile.Read(ref _projectedStale); + if (memo is not null && ReferenceEquals(memo.Source, snapshot) && memo.Projected.Status == projected) + { + return memo.Projected; + } + + GalaxyHierarchyCacheEntry created = snapshot with { Status = projected }; + Volatile.Write(ref _projectedStale, new ProjectedStale(snapshot, created)); + return created; } } @@ -355,7 +381,7 @@ public sealed class GalaxyHierarchyCache : IGalaxyHierarchyCache, IDisposable } if (snapshot.LastSuccessAt is { } success - && _timeProvider.GetUtcNow() - success > StaleThreshold) + && _timeProvider.GetUtcNow() - success > _staleThreshold) { return GalaxyCacheStatus.Stale; } diff --git a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyIndex.cs b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyIndex.cs index 3b1441a..86d8bb6 100644 --- a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyIndex.cs +++ b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyIndex.cs @@ -127,6 +127,8 @@ public sealed class GalaxyHierarchyIndex bucket.Add(view); } + ReRootUnreachable(views, childrenByParent); + foreach (List bucket in childrenByParent.Values) { bucket.Sort(CompareByAreaThenDisplayName); @@ -147,6 +149,109 @@ public sealed class GalaxyHierarchyIndex viewsByContainedPath); } + /// + /// Re-roots objects that are not reachable from key 0 — members of a multi-node parent + /// cycle (A→B→C→A, every parent present) and anything hanging off one — so they do not + /// vanish from browse. Each cycle is broken at its lowest gobject id. + /// + private static void ReRootUnreachable( + List views, + Dictionary> childrenByParent) + { + HashSet reached = []; + MarkReachable(0, childrenByParent, reached); + if (reached.Count >= views.Count) + { + return; + } + + // Ascending id: a cycle member with the lowest id is re-rooted before any other + // member; a descendant hanging off a cycle is reached once its cycle is re-rooted. + foreach (GalaxyObjectView view in views.OrderBy(v => v.Object.GobjectId)) + { + int id = view.Object.GobjectId; + if (reached.Contains(id)) + { + continue; + } + + // Only break at a node that is itself on a cycle (walking parents returns to it); + // a plain descendant of a cycle waits for its ancestor cycle to be broken. + if (!IsOnCycle(view, childrenByParent)) + { + continue; + } + + childrenByParent[view.Object.ParentGobjectId].Remove(view); + if (!childrenByParent.TryGetValue(0, out List? roots)) + { + roots = []; + childrenByParent[0] = roots; + } + + roots.Add(view); + MarkReachable(0, childrenByParent, reached); + } + } + + private static bool IsOnCycle( + GalaxyObjectView start, + Dictionary> childrenByParent) + { + // Descend from start through children; on a cycle we come back to start. + HashSet seen = []; + Stack stack = new(); + stack.Push(start.Object.GobjectId); + while (stack.Count > 0) + { + int key = stack.Pop(); + if (!childrenByParent.TryGetValue(key, out List? kids)) + { + continue; + } + + foreach (GalaxyObjectView kid in kids) + { + int kidId = kid.Object.GobjectId; + if (kidId == start.Object.GobjectId) + { + return true; + } + + if (seen.Add(kidId)) + { + stack.Push(kidId); + } + } + } + + return false; + } + + private static void MarkReachable( + int rootKey, + Dictionary> childrenByParent, + HashSet reached) + { + Stack stack = new(); + stack.Push(rootKey); + while (stack.Count > 0) + { + if (!childrenByParent.TryGetValue(stack.Pop(), out List? kids)) + { + continue; + } + + foreach (GalaxyObjectView kid in kids) + { + if (reached.Add(kid.Object.GobjectId)) + { + stack.Push(kid.Object.GobjectId); + } + } + } + } + private static string BuildContainedPath( GalaxyObject obj, IReadOnlyDictionary objectsById) diff --git a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchySnapshotStore.cs b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchySnapshotStore.cs index e3f74e9..c18fa0e 100644 --- a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchySnapshotStore.cs +++ b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchySnapshotStore.cs @@ -46,6 +46,13 @@ public sealed class GalaxyHierarchySnapshotStore : IGalaxyHierarchySnapshotStore : null; _writeTimeout = TimeSpan.FromSeconds(Math.Max(1, value.CommandTimeoutSeconds)); _logger = logger; + + if (value.PersistSnapshot && _path is null) + { + _logger?.LogInformation( + "Galaxy hierarchy snapshot persistence is disabled: PersistSnapshot is true but SnapshotCachePath is empty. " + + "Last-known browse data will not survive a restart while the Galaxy database is unreachable."); + } } /// diff --git a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyRepositoryOptions.cs b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyRepositoryOptions.cs index 35fa93b..aedf313 100644 --- a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyRepositoryOptions.cs +++ b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyRepositoryOptions.cs @@ -48,8 +48,9 @@ public sealed class GalaxyRepositoryOptions /// File path for the persisted Galaxy browse snapshot. Ignored when /// is . There is no built-in /// default path — the consumer supplies a cross-platform-friendly path appropriate to - /// its host. When left empty and is enabled, the - /// snapshot store (a later task) decides where to write. + /// its host. When left empty, snapshot persistence is disabled even if + /// is (the store logs this once at + /// Information), so the cold-start last-known-data guarantee does not apply. /// public string SnapshotCachePath { get; init; } = string.Empty; } diff --git a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/ZB.MOM.WW.GalaxyRepository.csproj b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/ZB.MOM.WW.GalaxyRepository.csproj index 4c9fbf3..bc0bcd7 100644 --- a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/ZB.MOM.WW.GalaxyRepository.csproj +++ b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/ZB.MOM.WW.GalaxyRepository.csproj @@ -12,6 +12,8 @@ + + @@ -22,7 +24,6 @@ - diff --git a/ZB.MOM.WW.GalaxyRepository/tests/ZB.MOM.WW.GalaxyRepository.Tests/ReviewP3Tests.cs b/ZB.MOM.WW.GalaxyRepository/tests/ZB.MOM.WW.GalaxyRepository.Tests/ReviewP3Tests.cs new file mode 100644 index 0000000..7553ba8 --- /dev/null +++ b/ZB.MOM.WW.GalaxyRepository/tests/ZB.MOM.WW.GalaxyRepository.Tests/ReviewP3Tests.cs @@ -0,0 +1,200 @@ +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using ZB.MOM.WW.GalaxyRepository.DependencyInjection; +using ZB.MOM.WW.GalaxyRepository.Grpc; + +namespace ZB.MOM.WW.GalaxyRepository.Tests; + +/// Regression tests for GalaxyRepository-003..008 (scadaproj#47). +public sealed class ReviewP3Tests +{ + private static GalaxyObject Obj(int id, int parent, string name) + { + GalaxyObject o = new() { GobjectId = id, ParentGobjectId = parent, ContainedName = name, BrowseName = name, TagName = name }; + return o; + } + + // ---- 003 ---- + [Fact] + public void ParentCycle_members_are_reachable_from_the_root_browse() + { + GalaxyObject[] objects = + [ + Obj(1, 0, "Plant"), + Obj(10, 12, "A"), + Obj(11, 10, "B"), + Obj(12, 11, "C"), + Obj(13, 12, "Leaf"), // hangs off the cycle + ]; + GalaxyHierarchyCacheEntry entry = GalaxyHierarchyCacheEntry.Empty with + { + Status = GalaxyCacheStatus.Healthy, + Sequence = 1, + LastSuccessAt = DateTimeOffset.UtcNow, + Objects = objects, + Index = GalaxyHierarchyIndex.Build(objects), + ObjectCount = objects.Length, + }; + + HashSet seen = []; + Walk(entry, new BrowseChildrenRequest(), seen, 0); + + Assert.Equal(new[] { "A", "B", "C", "Leaf", "Plant" }, seen.Order(StringComparer.Ordinal)); + } + + private static void Walk(GalaxyHierarchyCacheEntry entry, BrowseChildrenRequest req, HashSet seen, int depth) + { + Assert.True(depth < 20, "browse did not terminate"); + GalaxyBrowseChildrenResult r = GalaxyBrowseProjector.ProjectChildren(entry, req, null, 0, 100); + foreach (GalaxyObject child in r.Children) + { + if (seen.Add(child.TagName)) + { + Walk(entry, new BrowseChildrenRequest { ParentTagName = child.TagName }, seen, depth + 1); + } + } + } + + // ---- 004 ---- + [Fact] + public async Task SlowRefreshInterval_does_not_report_Stale_before_three_intervals() + { + DateTimeOffset now = new(2026, 1, 1, 12, 0, 0, TimeSpan.Zero); + FakeGalaxyRepository repo = new( + [new() { GobjectId = 1, TagName = "A", ContainedName = "A", BrowseName = "A" }], [], new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc)); + StubTimeProvider clock = new(now); + IOptions opts = Options.Create(new GalaxyRepositoryOptions { DashboardRefreshIntervalSeconds = 600 }); + using GalaxyHierarchyCache cache = new(repo, new RecordingDeployNotifier(), clock, options: opts); + await cache.RefreshAsync(CancellationToken.None); + + clock.Advance(TimeSpan.FromMinutes(10)); // one 600 s interval + Assert.Equal(GalaxyCacheStatus.Healthy, cache.Current.Status); + clock.Advance(TimeSpan.FromMinutes(21)); // > 3 intervals + Assert.Equal(GalaxyCacheStatus.Stale, cache.Current.Status); + } + + [Fact] + public async Task Stale_projection_returns_the_same_instance_so_projector_memos_stay_warm() + { + StubTimeProvider clock = new(new DateTimeOffset(2026, 1, 1, 12, 0, 0, TimeSpan.Zero)); + FakeGalaxyRepository repo = new( + [new() { GobjectId = 1, TagName = "A", ContainedName = "A", BrowseName = "A" }], [], new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc)); + using GalaxyHierarchyCache cache = new(repo, new RecordingDeployNotifier(), clock); + await cache.RefreshAsync(CancellationToken.None); + clock.Advance(TimeSpan.FromMinutes(6)); + + Assert.Equal(GalaxyCacheStatus.Stale, cache.Current.Status); + Assert.Same(cache.Current, cache.Current); + } + + // ---- 005 ---- + [Fact] + public async Task Subscriber_never_yields_an_older_event_after_a_newer_one() + { + GalaxyDeployNotifier notifier = new(); + GalaxyDeployEventInfo older = new(1, DateTimeOffset.UtcNow, DateTimeOffset.UtcNow, 1, 1); + GalaxyDeployEventInfo newer = new(2, DateTimeOffset.UtcNow, DateTimeOffset.UtcNow, 1, 1); + notifier.Publish(older); + + using CancellationTokenSource cts = new(); + await using IAsyncEnumerator e = notifier.SubscribeAsync(cts.Token).GetAsyncEnumerator(cts.Token); + Assert.True(await e.MoveNextAsync().AsTask().WaitAsync(TimeSpan.FromSeconds(2))); + Assert.Same(older, e.Current); + + // Simulate the race outcome: channel holds [newer, older] -> publish newer then replay older. + notifier.Publish(newer); + notifier.Publish(older); + Assert.True(await e.MoveNextAsync().AsTask().WaitAsync(TimeSpan.FromSeconds(2))); + Assert.Same(newer, e.Current); + + notifier.Publish(new GalaxyDeployEventInfo(3, DateTimeOffset.UtcNow, DateTimeOffset.UtcNow, 1, 1)); + Assert.True(await e.MoveNextAsync().AsTask().WaitAsync(TimeSpan.FromSeconds(2))); + Assert.Equal(3, e.Current.Sequence); // the stale replay of 1 was skipped + } + + // ---- 006 ---- + private sealed class MarkerScope : IGalaxyBrowseScopeProvider + { + public IReadOnlyList? ResolveBrowseSubtrees(global::Grpc.Core.ServerCallContext context) => ["x/*"]; + } + + private static IHost BuildHost(bool scopeBefore, bool scopeAfter, Dictionary? extra = null) + { + HostApplicationBuilder b = Host.CreateApplicationBuilder(); + Dictionary cfg = new() + { + ["Galaxy:ConnectionString"] = "Server=127.0.0.1,1;Database=ZB;User Id=x;Password=x;Connect Timeout=1;Encrypt=false", + ["Galaxy:DashboardRefreshIntervalSeconds"] = "1", + }; + if (extra is not null) { foreach (var kv in extra) cfg[kv.Key] = kv.Value; } + b.Configuration.AddInMemoryCollection(cfg); + b.Services.AddGrpc(); + if (scopeBefore) b.Services.AddSingleton(); + b.Services.AddZbGalaxyRepository(b.Configuration, "Galaxy"); + if (scopeAfter) b.Services.AddSingleton(); + return b.Build(); + } + + [Fact] + public async Task AddZbGalaxyRepository_builds_resolves_and_starts_and_stops_against_an_unreachable_db() + { + using IHost host = BuildHost(false, false); + IServiceProvider sp = host.Services; + Assert.NotNull(sp.GetRequiredService()); + Assert.NotNull(sp.GetRequiredService()); + Assert.NotNull(sp.GetRequiredService()); + IGalaxyHierarchyCache cache = sp.GetRequiredService(); + Assert.Same(cache, sp.GetRequiredService()); + Assert.Contains(sp.GetServices(), s => s is GalaxyHierarchyRefreshService); + Assert.NotNull(ActivatorUtilities.CreateInstance(sp)); + Assert.IsType(sp.GetRequiredService()); + + await host.StartAsync(); + using CancellationTokenSource cts = new(TimeSpan.FromSeconds(30)); + await cache.WaitForFirstLoadAsync(cts.Token); + Assert.Equal(GalaxyCacheStatus.Unavailable, cache.Current.Status); + await host.StopAsync(); + } + + [Fact] + public void Host_registered_scope_provider_wins_when_registered_before_the_call() + { + using IHost host = BuildHost(scopeBefore: true, scopeAfter: false); + Assert.IsType(host.Services.GetRequiredService()); + } + + [Fact] + public void Host_registered_scope_provider_wins_when_registered_after_the_call() + { + using IHost host = BuildHost(scopeBefore: false, scopeAfter: true); + Assert.IsType(host.Services.GetRequiredService()); + } + + // ---- 008 ---- + private sealed class CapturingLogger : ILogger + { + public List<(LogLevel Level, string Message)> Entries { get; } = []; + public IDisposable? BeginScope(TState state) where TState : notnull => null; + public bool IsEnabled(LogLevel logLevel) => true; + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func formatter) => + Entries.Add((logLevel, formatter(state, exception))); + } + + [Fact] + public void Snapshot_store_logs_once_when_persistence_is_enabled_but_path_is_empty() + { + CapturingLogger log = new(); + using GalaxyHierarchySnapshotStore _ = new( + Options.Create(new GalaxyRepositoryOptions { PersistSnapshot = true, SnapshotCachePath = "" }), log); + Assert.Single(log.Entries, e => e.Level == LogLevel.Information && e.Message.Contains("SnapshotCachePath")); + + CapturingLogger quiet = new(); + using GalaxyHierarchySnapshotStore __ = new( + Options.Create(new GalaxyRepositoryOptions { PersistSnapshot = false }), quiet); + Assert.Empty(quiet.Entries); + } +} diff --git a/code-reviews/GalaxyRepository/findings.md b/code-reviews/GalaxyRepository/findings.md index d0960cc..772c1c2 100644 --- a/code-reviews/GalaxyRepository/findings.md +++ b/code-reviews/GalaxyRepository/findings.md @@ -10,7 +10,7 @@ | Last reviewed | 2026-11-09 | | Reviewer | Claude | | Commit reviewed | `91fb2b4` | -| Open findings | 6 | +| Open findings | 0 | ## Summary @@ -171,7 +171,7 @@ Resolved 2026-11-09 in the commit tagged `(scadaproj#42)` (GalaxyRepository `0.2 | Severity | Low | | Priority | P3 | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyIndex.cs:106` | **Description** @@ -203,7 +203,7 @@ reached (break each cycle at its lowest gobject id) and log a warning naming the **Resolution** -_Unresolved._ +Resolved 2026-11-09 in the commit tagged `(scadaproj#47)` (GalaxyRepository `0.2.2`, unpublished) — `GalaxyHierarchyIndex.Build` now walks reachability from key 0 after bucketing and re-roots any node that lies on a parent cycle (lowest gobject id first; nodes hanging off the cycle become reachable with it). Regression test `ReviewP3Tests.ParentCycle_members_are_reachable_from_the_root_browse` (A→B→C→A plus a leaf; failed without the fix). Not done: the recommended warning log (the index is a static pure builder with no logger). ### GalaxyRepository-004 — Stale threshold is hard-coded at 5 min regardless of the refresh interval, and stale projection defeats the memo @@ -212,7 +212,7 @@ _Unresolved._ | Severity | Low | | Priority | P3 | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyCache.cs:19` (projection at `:55-64`, `:357`) | **Description** @@ -244,7 +244,7 @@ instance, so a status-only projection does not invalidate it. **Resolution** -_Unresolved._ +Resolved 2026-11-09 in the commit tagged `(scadaproj#47)` (GalaxyRepository `0.2.2`, unpublished) — the threshold is now `max(5 min, 3 x DashboardRefreshIntervalSeconds)`, read from an optional `IOptions` constructor parameter (DI supplies it; existing callers keep the 5 min floor). The projected-Stale record is memoized per source entry, so `Current` returns the same instance across reads and the projector memos stay warm. Tests `SlowRefreshInterval_does_not_report_Stale_before_three_intervals` and `Stale_projection_returns_the_same_instance_so_projector_memos_stay_warm`. ### GalaxyRepository-005 — Deploy-notifier bootstrap can deliver an older event after a newer one @@ -253,7 +253,7 @@ _Unresolved._ | Severity | Low | | Priority | P3 | | Category | Concurrency & thread safety | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyDeployNotifier.cs:53` | **Description** @@ -286,7 +286,7 @@ duplicate case, where the same event arrives both via `Publish` and via bootstra **Resolution** -_Unresolved._ +Resolved 2026-11-09 in the commit tagged `(scadaproj#47)` (GalaxyRepository `0.2.2`, unpublished) — took the reader-side option: `SubscribeAsync` skips any event whose `Sequence` is not greater than the last one yielded, which also drops the duplicate-delivery case (reading `_latest` before registering would instead lose an event published in between). Test `Subscriber_never_yields_an_older_event_after_a_newer_one` (failed without the fix). ### GalaxyRepository-006 — No test builds the container from `AddZbGalaxyRepository` @@ -295,7 +295,7 @@ _Unresolved._ | Severity | Low | | Priority | P3 | | Category | Testing coverage | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/DependencyInjection/GalaxyRepositoryServiceCollectionExtensions.cs:31` | **Description** @@ -332,7 +332,7 @@ Add container-building tests that: **Resolution** -_Unresolved._ +Resolved 2026-11-09 in the commit tagged `(scadaproj#47)` (GalaxyRepository `0.2.2`, unpublished) — new container-building tests in `ReviewP3Tests`: a real `Host` with `AddGrpc()` + `AddZbGalaxyRepository` resolves the repository, notifier, snapshot store, cache (singleton), hosted refresh service and (via `ActivatorUtilities`) `GalaxyRepositoryGrpcService`; starts and stops against an unreachable SQL Server with the cache settling to `Unavailable`; and a host `IGalaxyBrowseScopeProvider` registered before or after the call is the one resolved. Guard test, no behaviour change. ### GalaxyRepository-007 — Shipped package pulls a vulnerable `System.Security.Cryptography.Pkcs` 9.0.4 @@ -341,7 +341,7 @@ _Unresolved._ | Severity | Low | | Priority | P3 | | Category | Packaging, dependencies & project layout | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.GalaxyRepository/Directory.Packages.props:9` | **Description** @@ -365,7 +365,7 @@ version. Publish this as a patch release. **Resolution** -_Unresolved._ +Resolved 2026-11-09 in the commit tagged `(scadaproj#47)` (GalaxyRepository `0.2.2`, unpublished) — `System.Security.Cryptography.Pkcs` pinned to the patched `9.0.21` in `Directory.Packages.props` and referenced directly from the library csproj (as scadaproj#22 did for Secrets/Auth). Verified: restore resolves `Pkcs/9.0.21` with no NU1902/NU1903. Consumers see the fix only after the `0.2.2` release is published. ### GalaxyRepository-008 — Stale documentation: `SnapshotCachePath` default, csproj comment, consumer matrix @@ -374,7 +374,7 @@ _Unresolved._ | Severity | Low | | Priority | P3 | | Category | Documentation & XML docs | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyRepositoryOptions.cs:47` | **Description** @@ -406,4 +406,4 @@ silently does not exist on that host, and nothing logs that persistence is off. **Resolution** -_Unresolved._ +Resolved 2026-11-09 in the commit tagged `(scadaproj#47)` (GalaxyRepository `0.2.2`, unpublished) — `SnapshotCachePath` XML doc now says an empty path disables persistence; `GalaxyHierarchySnapshotStore` logs once at Information when `PersistSnapshot` is true but the path is empty (test `Snapshot_store_logs_once_when_persistence_is_enabled_but_path_is_empty`); stale csproj comment removed; CLAUDE.md consumer matrix updated (HistorianGateway + mxaccessgw). diff --git a/code-reviews/README.md b/code-reviews/README.md index 18a2112..15a22ad 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -40,7 +40,7 @@ code-reviews/ ## Summary -10 of 10 libraries reviewed. 44 pending findings across all libraries. +10 of 10 libraries reviewed. 38 pending findings across all libraries. Coverage: Secrets, LocalDb, GalaxyRepository and Overview first reviewed, and Auth, Health, Theme, Audit, Configuration and Telemetry re-reviewed, on 2026-11-09 (#15). Each finding also @@ -51,8 +51,8 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low). | Critical | 0 | | High | 0 | | Medium | 11 | -| Low | 33 | -| **Total** | **44** | +| Low | 27 | +| **Total** | **38** | ## Library Status @@ -61,7 +61,7 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low). | [Audit](Audit/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 5 | | [Auth](Auth/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/1/3 | 4 | 10 | | [Configuration](Configuration/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 4 | -| [GalaxyRepository](GalaxyRepository/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/6 | 6 | 8 | +| [GalaxyRepository](GalaxyRepository/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 8 | | [Health](Health/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/1 | 1 | 7 | | [LocalDb](LocalDb/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/4/3 | 7 | 7 | | [Overview](Overview/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/3 | 3 | 5 | @@ -100,19 +100,13 @@ _None open._ | Secrets-013 | [Secrets](Secrets/findings.md) | UI add/rotate/delete: if the audit write throws after the store write succeeded, the change is reported and audited as a failure and the resolver cache is not evicted | | Secrets-017 | [Secrets](Secrets/findings.md) | `secret set`/`rotate` take the plaintext on argv; the host builder re-parses argv as configuration and can crash with the value in the error | -### Low (33) +### Low (27) | ID | Library | Title | |----|---------|-------| | Auth-008 | [Auth](Auth/findings.md) | SQL Server store silently truncates over-length values and matches key ids case-insensitively, diverging from SQLite | | Auth-009 | [Auth](Auth/findings.md) | Canonical username is the first value of a possibly multi-valued attribute, not the value the user matched | | Auth-010 | [Auth](Auth/findings.md) | `ApiKeyVerifier` lets store exceptions escape despite its "only exception path is cancellation" contract | -| GalaxyRepository-003 | [GalaxyRepository](GalaxyRepository/findings.md) | Objects in a parent cycle disappear from `BrowseChildren` | -| GalaxyRepository-004 | [GalaxyRepository](GalaxyRepository/findings.md) | Stale threshold is hard-coded at 5 min regardless of the refresh interval, and stale projection defeats the memo | -| GalaxyRepository-005 | [GalaxyRepository](GalaxyRepository/findings.md) | Deploy-notifier bootstrap can deliver an older event after a newer one | -| GalaxyRepository-006 | [GalaxyRepository](GalaxyRepository/findings.md) | No test builds the container from `AddZbGalaxyRepository` | -| GalaxyRepository-007 | [GalaxyRepository](GalaxyRepository/findings.md) | Shipped package pulls a vulnerable `System.Security.Cryptography.Pkcs` 9.0.4 | -| GalaxyRepository-008 | [GalaxyRepository](GalaxyRepository/findings.md) | Stale documentation: `SnapshotCachePath` default, csproj comment, consumer matrix | | Health-007 | [Health](Health/findings.md) | `AkkaActiveNodeGate` cannot express the probe's `RolePreference`, so gate and `/health/active` can disagree despite the documented guarantee | | LocalDb-005 | [LocalDb](LocalDb/findings.md) | Auto-assigned rowid / AUTOINCREMENT keys collide across nodes and LWW silently drops one row | | LocalDb-006 | [LocalDb](LocalDb/findings.md) | Column names containing `'` are captured but break the apply and snapshot SQL |