scadaproj/code-reviews/GalaxyRepository/findings.md

Code Review — GalaxyRepository

Field	Value
Library	ZB.MOM.WW.GalaxyRepository/
Packages	ZB.MOM.WW.GalaxyRepository
Component spec	None under components/. The design of record is docs/plans/2026-06-23-historian-gateway-design.md §10 (component 1).
Shared contract	ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/Protos/galaxy_repository.proto (galaxy_repository.v1, wire-compatible with mxaccessgw's original)
Status	Reviewed
Last reviewed	2026-11-09
Reviewer	Claude
Commit reviewed	91fb2b4
Open findings	0

Summary

The library is in good shape overall. All SQL is constant text with no string-built input, so there is no injection surface. Every SqlConnection, SqlCommand and SqlDataReader is disposed. The cache publishes immutable entries through Volatile reference swaps behind a single refresh gate. The refresh BackgroundService cannot fault the host. The snapshot store writes atomically (temp file + rename) and fails soft. Page tokens are bound to the cache sequence and to a filter signature.

The two Medium findings are in the browse projectors:

• GalaxyRepository-001: the per-entry filter memo has no size limit. Any gRPC client can grow it to gigabytes until the next Galaxy deploy. A scratch test retained 1.0 GB from 2000 distinct tag_name_glob values over 50k objects.
• GalaxyRepository-002: browse-subtree scoping hides the ancestors of an allowed subtree. A scoped API key that browses lazily from the root gets an empty tree.

The remaining findings are Low: robustness, testing, packaging and docs.

AddZbGalaxyRepository was checked against the family's DI-defect pattern. A scratch test built a real Host with AddGrpc() + AddZbGalaxyRepository, resolved every service, started the host against an unreachable SQL Server and observed the cache settle to Unavailable. No wiring defect was found, but the repo has no such test (GalaxyRepository-006).

The existing suite passes: 64/64 tests. The consumer side was spot-checked. mxaccessgw's GatewayBrowseScopeProvider registers before the library's TryAddSingleton default and fails closed when identity is absent, so the empty-means-unscoped contract is handled correctly there.

Checklist coverage

#	Category	Examined	Notes
1	Correctness & logic bugs	☑	Hierarchy CTEs carry a depth-10 guard on template/package chains. Orphans and self-parents are re-rooted. See GalaxyRepository-002 (scoped ancestors hidden) and GalaxyRepository-003 (parent cycles vanish from browse). Deploy detection is gated on time_of_last_deploy and self-heals on the next tick after a mid-pull deploy.
2	Public API surface & compatibility	☑	Proto is unchanged and wire-compatible. IGalaxyBrowseScopeProvider treats null/empty as "no restriction" (fail-open by contract). The one consumer that implements it (mxaccessgw) handles this correctly. No issues found.
3	Concurrency & thread safety	☑	Immutable entries, a refresh SemaphoreSlim, and a bounded regex cache with locked eviction. See GalaxyRepository-005 (deploy-notifier bootstrap race reorders events).
4	Error handling & resilience	☑	Cache refresh catches every non-cancellation exception and degrades to Stale/Unavailable. The snapshot store fails soft. See GalaxyRepository-004 (hard-coded 5-min stale threshold).
5	Security & secret handling	☑	No string-built SQL. Regex has a 100 ms match timeout and a bounded compile cache. See GalaxyRepository-001 (unbounded memo is client-driven memory exhaustion).
6	Performance & resource management	☑	All ADO.NET objects are disposed. See GalaxyRepository-001 and GalaxyRepository-004 (the memo is defeated while the projected status is Stale).
7	Spec & shared-contract adherence	☑	Matches the design §10 split: SQL provider, cache, snapshot, deploy poll, projector and DI in the lib; scoping in the consumer. No issues found.
8	Packaging, dependencies & project layout	☑	See GalaxyRepository-007: the transitive System.Security.Cryptography.Pkcs 9.0.4 has a high-severity advisory (NU1903).
9	Testing coverage	☑	Projectors, cache, notifier, snapshot store and gRPC scoping are covered. No container-building test exists for the DI extension (GalaxyRepository-006). No test covers a scoped browse from the root (GalaxyRepository-002).
10	Documentation & XML docs	☑	See GalaxyRepository-008 (stale SnapshotCachePath doc, csproj comment and CLAUDE.md consumer matrix).

Findings

GalaxyRepository-001 — Filter memo has no size limit: client-chosen filters retain unlimited memory until the next deploy

Severity	Medium
Priority	P2
Category	Performance & resource management
Status	Resolved
Location	ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyProjector.cs:28 (memo insert :117); same pattern in ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyBrowseProjector.cs:20 (insert :121)

Description

GalaxyHierarchyProjector keeps a ConditionalWeakTable<GalaxyHierarchyCacheEntry, ConcurrentDictionary<string, IReadOnlyList<GalaxyObjectView>>>. Every distinct filter signature gets its own entry: root, max depth, category ids, template filters, tag_name_glob, attribute/alarm/historized flags and browse globs. Each entry holds the full list of matching views, so a broad filter costs one reference per Galaxy object.

Nothing removes entries. The dictionary is released only when the cache publishes a new entry, which happens only when galaxy.time_of_last_deploy advances. In production that can be days or weeks apart. GalaxyBrowseProjector.FilteredChildrenCache has the same shape.

tag_name_glob is fully client-controlled. *, **, *** and so on are all distinct signatures that match everything. The authors already treated client globs as hostile in GalaxyGlobMatcher (its regex cache is deliberately capped at 256 for exactly this reason). The memo one layer up received no such limit.

Failure scenario

Take a gateway (mxaccessgw or HistorianGateway) serving a 50k-object Galaxy. A client with a valid API key, or a buggy client that builds a fresh glob per call, issues DiscoverHierarchy with 2000 distinct tag_name_glob values and page_size=1.

A scratch test of exactly this (Project(entry, {TagNameGlob = new string('*', i)}, null, 0, 1) for i = 1..2000) retained 1010 MB. Memory keeps growing with every new glob until the next Galaxy deploy. The gateway process (the x64 host that also brokers MXAccess or Historian traffic) can be driven out of memory without a single SQL query.

Recommendation

Bound the memo per entry. Options:

• An LRU or FIFO cap (e.g. 64 signatures).
• Memoize only when the request carries a page_token or is likely to page, i.e. when TotalObjectCount exceeds the page size.

Apply the same cap to GalaxyBrowseProjector. Add a test that drives more than the cap of distinct globs and asserts the memo size.

Resolution

Resolved 2026-11-09 in the commit tagged (scadaproj#41) (GalaxyRepository 0.2.1, unpublished) — new internal ProjectionMemo<T> (lock + dictionary + FIFO queue, Capacity = 64, factory runs outside the lock) replaces the unbounded ConcurrentDictionary in both GalaxyHierarchyProjector.FilteredViewCache and GalaxyBrowseProjector.FilteredChildrenCache. Eviction is safe because the projection is a pure function of (entry, request): an evicted signature is recomputed with the same result and page-token signature. Regression tests in ProjectionMemoBoundTests drive 500 distinct tag_name_glob values through each projector and assert the memo stays within the cap (both failed without the fix: 500 retained), plus an eviction-then-requery equality test.

GalaxyRepository-002 — Browse-subtree scoping hides the ancestors of an allowed subtree, so scoped lazy browse from the root is empty

Severity	Medium
Priority	P2
Category	Correctness & logic bugs
Status	Resolved
Location	ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyBrowseProjector.cs:134 (also ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyProjector.cs:233)

Description

GetFilteredChildren runs MatchesBrowseSubtrees(view, globs) first and continues when a direct child fails it. It never checks whether that child has a descendant that is inside the allowed subtree.

Right below, the request-filter branch does surface ancestors of matching descendants, with HasMatchingDescendant and the comment "a matching descendant should still surface its ancestor". The scope branch has no equivalent.

So with a scope glob like Plant/LineA/*, neither Plant nor Plant/LineA matches. Both are filtered out of their parent's child list. DiscoverHierarchy likewise omits the Plant/LineA area object itself.

Failure scenario

An mxaccessgw API key is scoped to BrowseSubtrees: ["Plant/LineA/*"]. A lazy-browse client starts at the root with BrowseChildren{} and expands nodes, as the OtOpcUa / ScadaBridge browse dialogs do. Scratch-test result over Plant → LineA → Pump01, Plant → LineB → Pump02:

• Root BrowseChildren: TotalChildCount = 0.
• DiscoverHierarchy: only Pump01 (no LineA area).
• BrowseChildren{ParentContainedPath="Plant/LineA"}: 1 child.

The scoped user sees an empty Galaxy unless they already know and type the exact parent path.

Admins can work around it by also listing the ancestor paths as globs (Plant, Plant/LineA), but that is undocumented. It also exposes those ancestor objects with their attributes, which is wider than the intended scope.

Recommendation

In GetFilteredChildren, when a child fails the scope check, test whether it is an ancestor of an allowed path. Either use HasMatchingDescendant with the scope applied, or do a cheap prefix test of the child's ContainedPath + "/" against each glob's literal prefix. If it is an ancestor, include it as a navigation-only node (has_children=true) with its attributes stripped.

Document the semantics on IGalaxyBrowseScopeProvider. Add a test that browses from the root under a deep scope glob.

Resolution

Resolved 2026-11-09 in the commit tagged (scadaproj#42) (GalaxyRepository 0.2.1, unpublished) — an out-of-scope child that has an allowed descendant (also passing the request filters, via the existing HasMatchingDescendant) is now returned by BrowseChildren as a navigation-only container: child_has_children = true, attributes stripped; its other children stay hidden because the same rule applies when it is expanded. DiscoverHierarchy likewise returns the out-of-scope ancestors of allowed objects (cycle-safe parent walk; only when inside root/depth and passing the request filters), attribute-stripped, and reports them in the new GalaxyHierarchyQueryResult.NavigationAncestorCount init property, which WatchDeployEvents subtracts so its scoped counts remain allowed-objects-only. Semantics documented on IGalaxyBrowseScopeProvider. Regression tests in ScopedAncestorBrowseTests (root browse, ancestor browse, allowed subtree, sibling LineB does not leak, filter-gated ancestor, scoped and unscoped DiscoverHierarchy); root/ancestor/DiscoverHierarchy tests fail without the fix. GalaxyRepositoryGrpcServiceScopeTests.WatchDeployEvents_ScopedProvider_EmitsFilteredCounts updated (projection 4 objects incl. 2 ancestors; event still 2). Behaviour change for consumers: a scoped DiscoverHierarchy now includes the ancestor objects.

GalaxyRepository-003 — Objects in a parent cycle disappear from BrowseChildren

Severity	Low
Priority	P3
Category	Correctness & logic bugs
Status	Resolved
Location	ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyIndex.cs:106

Description

GalaxyHierarchyIndex.Build re-roots two kinds of object under key 0: self-parented rows, and orphans whose parent is absent. The comment there says it does so "so nothing disappears from browse".

A multi-node cycle (A→B→C→A, every parent present) is not re-rooted. Each member is bucketed under another cycle member, and none is reachable from key 0. GalaxyBrowseProjector.HasMatchingDescendant already defends against such cycles, which shows the authors expected them to occur, but only for termination, not for reachability.

Failure scenario

Corrupt or mid-edit Galaxy data produces a containment cycle. A scratch test with Plant (root) plus a cycle A(10)→B(11)→C(12)→A gave:

• Root BrowseChildren: only Plant.
• DiscoverHierarchy: all 4 objects.

The three cycle objects, and their attributes, are unreachable for every lazy-browse client while still being present in flat discovery.

Recommendation

After bucketing, walk from key 0 and collect the reachable ids. Re-root any object that was not reached (break each cycle at its lowest gobject id) and log a warning naming the cycle.

Resolution

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

Severity	Low
Priority	P3
Category	Error handling & resilience
Status	Resolved
Location	ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyCache.cs:19 (projection at :55-64, :357)

Description

StaleThreshold is a static readonly 5 minutes. It is independent of GalaxyRepositoryOptions.DashboardRefreshIntervalSeconds, which is configurable with no upper bound.

When now - LastSuccessAt > 5 min, the Current getter returns snapshot with { Status = Stale }. That is a new record instance on every read. Both projectors key their memo on the entry instance (ConditionalWeakTable), so every page of every request misses the memo and re-filters the full object list (O(total) per page). Each read also attaches a throwaway memo dictionary.

Failure scenario

A consumer sets DashboardRefreshIntervalSeconds: 600 to reduce SQL load. For half of every interval, the healthy cache reports Stale to the dashboard and to GetLastDeployTime callers. Every DiscoverHierarchy page meanwhile does full re-filtering.

A scratch test confirmed it: after advancing the clock 6 min past a successful refresh, two consecutive cache.Current reads returned Status=Stale and ReferenceEquals == false. The same memo bypass happens in the legitimate stale case (SQL unreachable for more than 5 min). That is when the gateway is already degraded.

Recommendation

Derive the threshold from the options, e.g. max(5 min, 3 × refresh interval), or make it an option. Cache the projected-stale record, or key the memo on Sequence rather than on the record instance, so a status-only projection does not invalidate it.

Resolution

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<GalaxyRepositoryOptions> 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

Severity	Low
Priority	P3
Category	Concurrency & thread safety
Status	Resolved
Location	ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyDeployNotifier.cs:53

Description

SubscribeAsync takes these steps in order:

1. Registers the channel in _subscribers (:53).
2. Reads _latest (:57).
3. Writes that bootstrap value into the channel (:60).

Publish can run between steps 2 and 3. It sets _latest = new, iterates _subscribers (which already contains this channel) and TryWrite(new). The subscriber then writes the stale old it read in step 2. The channel holds [new, old].

Failure scenario

A client opens WatchDeployEvents at the moment a refresh publishes a redeploy. It receives sequence N+1 followed by sequence N. A client that treats the most recent event as current state ends up on the older deploy time. If it sent last_seen_deploy_time = new it may also have the new event suppressed and then receive old as if it were a change, triggering a spurious rebuild.

The window is small (microseconds), but it is a real ordering violation, and the class doc tells clients to use sequence to detect gaps.

Recommendation

Read _latest before registering the subscriber. Alternatively, have the reader skip any event whose Sequence is not greater than the last one yielded. The second option also covers the duplicate case, where the same event arrives both via Publish and via bootstrap.

Resolution

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

Severity	Low
Priority	P3
Category	Testing coverage
Status	Resolved
Location	ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/DependencyInjection/GalaxyRepositoryServiceCollectionExtensions.cs:31

Description

No test calls AddZbGalaxyRepository or MapZbGalaxyRepository. Every test constructs the types by hand. This family has shipped three DI-extension defects of exactly this class: the Secrets TryAdd-order no-op, the factory-lambda singleton cycle and the inert replicator. Each was invisible without a test that builds the provider.

The extension also carries a non-obvious ordering contract: the host must register its IGalaxyBrowseScopeProvider before the call so that the TryAddSingleton default does not win. mxaccessgw relies on this and documents it at GatewayApplication.cs:152.

Failure scenario

A future edit swaps a registration to TryAdd, introduces a factory cycle, or changes the scope provider from TryAdd to Add. If the scope provider changes to Add, NullGalaxyBrowseScopeProvider would override a host provider registered earlier and silently un-scope every API key. Nothing in this repo's suite would fail.

A scratch test confirms the current wiring is correct: it builds a Host with AddGrpc() + AddZbGalaxyRepository, resolves all services, starts the host and sees the cache reach Unavailable. So this finding is a missing guard, not a present bug.

Recommendation

Add container-building tests that:

• Build the host and resolve IGalaxyHierarchyCache, IGalaxyRepository, the hosted service and GalaxyRepositoryGrpcService (via ActivatorUtilities).
• Assert that a host-registered IGalaxyBrowseScopeProvider (registered before and after the call) is the one resolved.
• Start and stop the host against an unreachable connection string.

Resolution

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

Severity	Low
Priority	P3
Category	Packaging, dependencies & project layout
Status	Resolved
Location	ZB.MOM.WW.GalaxyRepository/Directory.Packages.props:9

Description

Microsoft.Data.SqlClient 6.0.2 brings in System.Security.Cryptography.Pkcs 9.0.4 transitively. Restore reports NU1902 (GHSA-2fmv-rv2c-59x7, moderate) and NU1903 (GHSA-846f-f764-76mx, high) against both the library and anything that references it. This was observed while building a scratch consumer. The library has no transitive pin, so every consumer inherits the advisory.

Failure scenario

A consumer that builds with TreatWarningsAsErrors (the family's zero-warnings gate) fails to restore or build after a GalaxyRepository bump. It has to add its own pin, as already happened in HistorianGateway ("crypto pin"). Consumers that do not gate ship the vulnerable assembly.

Recommendation

Pin System.Security.Cryptography.Pkcs to a patched version in Directory.Packages.props and reference it from the library csproj, or move to a SqlClient release that depends on a patched version. Publish this as a patch release.

Resolution

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

Severity	Low
Priority	P3
Category	Documentation & XML docs
Status	Resolved
Location	ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyRepositoryOptions.cs:47

Description

Three places are out of date:

• SnapshotCachePath XML doc (GalaxyRepositoryOptions.cs:47-54). It says that when the path is empty and PersistSnapshot is enabled, "the snapshot store (a later task) decides where to write". The shipped GalaxyHierarchySnapshotStore (GalaxyHierarchySnapshotStore.cs:44-46) instead disables persistence entirely.
• Library csproj (ZB.MOM.WW.GalaxyRepository.csproj:22). It still says "Proto files are added in Task 2; the empty glob is intentional".
• ZB.MOM.WW.GalaxyRepository/CLAUDE.md. It says "mxaccessgw adoption is a tracked follow-on", but mxaccessgw has adopted the library: its GatewayApplication.cs calls AddZbGalaxyRepository / MapZbGalaxyRepository, and the root index records adoption as of 2026-07-18.

Failure scenario

An operator reads the option doc and leaves SnapshotCachePath empty, expecting a default location. They get no snapshot at all. The cold-start "browse last-known data while SQL is down" guarantee silently does not exist on that host, and nothing logs that persistence is off.

Recommendation

• Correct the XML doc to "empty disables persistence", and log once at Information when the store is disabled while PersistSnapshot is true.
• Drop the stale csproj comment.
• Update the CLAUDE.md consumer matrix (HistorianGateway + mxaccessgw).

Resolution

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).