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

## 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 | Open |
| 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**

_Unresolved._

### 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 | Open |
| 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 `continue`s 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**

_Unresolved._

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

| | |
|--|--|
| Severity | Low |
| Priority | P3 |
| Category | Correctness & logic bugs |
| Status | Open |
| 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**

_Unresolved._

### 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 | Open |
| 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**

_Unresolved._

### GalaxyRepository-005 — Deploy-notifier bootstrap can deliver an older event after a newer one

| | |
|--|--|
| Severity | Low |
| Priority | P3 |
| Category | Concurrency & thread safety |
| Status | Open |
| 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**

_Unresolved._

### GalaxyRepository-006 — No test builds the container from `AddZbGalaxyRepository`

| | |
|--|--|
| Severity | Low |
| Priority | P3 |
| Category | Testing coverage |
| Status | Open |
| 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**

_Unresolved._

### GalaxyRepository-007 — Shipped package pulls a vulnerable `System.Security.Cryptography.Pkcs` 9.0.4

| | |
|--|--|
| Severity | Low |
| Priority | P3 |
| Category | Packaging, dependencies & project layout |
| Status | Open |
| 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**

_Unresolved._

### GalaxyRepository-008 — Stale documentation: `SnapshotCachePath` default, csproj comment, consumer matrix

| | |
|--|--|
| Severity | Low |
| Priority | P3 |
| Category | Documentation & XML docs |
| Status | Open |
| 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**

_Unresolved._