diff --git a/ZB.MOM.WW.GalaxyRepository/CLAUDE.md b/ZB.MOM.WW.GalaxyRepository/CLAUDE.md index 3cd96a7..6a03cd8 100644 --- a/ZB.MOM.WW.GalaxyRepository/CLAUDE.md +++ b/ZB.MOM.WW.GalaxyRepository/CLAUDE.md @@ -2,6 +2,12 @@ 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.1 (2026-11-09, UNPUBLISHED) + +Version bumped in `Directory.Build.props`, not packed or pushed; consumers stay on `0.2.0`. +- GalaxyRepository-001 (scadaproj#41): the per-cache-entry filter memos of both projectors are now + `ProjectionMemo`, capped at 64 signatures (FIFO; an evicted signature is recomputed, same answer). + ## Index history (migrated from the scadaproj root CLAUDE.md) > Moved verbatim out of the scadaproj root `CLAUDE.md` on 2026-09-03 so it loads only when needed. **This file is now the authoritative place for this history — append new entries here and keep the root index row short.** diff --git a/ZB.MOM.WW.GalaxyRepository/Directory.Build.props b/ZB.MOM.WW.GalaxyRepository/Directory.Build.props index 87f19c3..48488ee 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.0 + 0.2.1 true true diff --git a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyBrowseProjector.cs b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyBrowseProjector.cs index cf0b05b..1ec3298 100644 --- a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyBrowseProjector.cs +++ b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyBrowseProjector.cs @@ -1,4 +1,3 @@ -using System.Collections.Concurrent; using System.Runtime.CompilerServices; using System.Security.Cryptography; using System.Text; @@ -13,13 +12,19 @@ namespace ZB.MOM.WW.GalaxyRepository; /// filtered child list per cache-entry instance so repeated paging is an O(pageSize) /// slice rather than an O(siblings) filter scan per page. The memo is keyed on the /// immutable cache entry, so when the cache publishes a new entry the stale memo -/// becomes unreachable and is reclaimed with it. +/// becomes unreachable and is reclaimed with it. Each entry's memo is capped at +/// signatures, because the signature is +/// client-controlled (GalaxyRepository-001). /// public static class GalaxyBrowseProjector { private static readonly ConditionalWeakTable< GalaxyHierarchyCacheEntry, - ConcurrentDictionary> FilteredChildrenCache = new(); + ProjectionMemo> FilteredChildrenCache = new(); + + /// Number of filter signatures currently memoized for (tests). + internal static int MemoCount(GalaxyHierarchyCacheEntry entry) => + FilteredChildrenCache.TryGetValue(entry, out var memo) ? memo.Count : 0; /// Projects one page of direct children of the resolved parent. /// The Galaxy hierarchy cache entry to query. @@ -115,8 +120,8 @@ public static class GalaxyBrowseProjector int parentId, string filterSignature) { - ConcurrentDictionary memo = - FilteredChildrenCache.GetValue(entry, static _ => new ConcurrentDictionary(StringComparer.Ordinal)); + ProjectionMemo memo = + FilteredChildrenCache.GetValue(entry, static _ => new ProjectionMemo()); return memo.GetOrAdd( filterSignature, diff --git a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyProjector.cs b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyProjector.cs index 27042c4..67d6449 100644 --- a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyProjector.cs +++ b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyProjector.cs @@ -1,4 +1,3 @@ -using System.Collections.Concurrent; using System.Runtime.CompilerServices; using System.Security.Cryptography; using System.Text; @@ -23,9 +22,15 @@ public static class GalaxyHierarchyProjector /// 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. + /// reclaimed with it — no explicit invalidation needed. Each entry's memo is capped at + /// signatures, because the signature is + /// client-controlled (GalaxyRepository-001). /// - private static readonly ConditionalWeakTable>> FilteredViewCache = new(); + private static readonly ConditionalWeakTable>> FilteredViewCache = new(); + + /// Number of filter signatures currently memoized for (tests). + internal static int MemoCount(GalaxyHierarchyCacheEntry entry) => + FilteredViewCache.TryGetValue(entry, out var memo) ? memo.Count : 0; /// Projects a discovery request against a cache entry and returns all matching objects. /// The Galaxy hierarchy cache entry. @@ -111,8 +116,8 @@ public static class GalaxyHierarchyProjector IReadOnlyList views = entry.Index.ObjectViews; GalaxyObjectView? root = ResolveRoot(request, entry.Index); - ConcurrentDictionary> memo = - FilteredViewCache.GetValue(entry, static _ => new ConcurrentDictionary>(StringComparer.Ordinal)); + ProjectionMemo> memo = + FilteredViewCache.GetValue(entry, static _ => new ProjectionMemo>()); return memo.GetOrAdd( filterSignature, diff --git a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/ProjectionMemo.cs b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/ProjectionMemo.cs new file mode 100644 index 0000000..021e684 --- /dev/null +++ b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/ProjectionMemo.cs @@ -0,0 +1,74 @@ +namespace ZB.MOM.WW.GalaxyRepository; + +/// +/// A small, bounded, thread-safe memo of filter-signature → projected result, held per +/// immutable . +/// +/// +/// Bounded because the signature is client-controlled: tag_name_glob values such as +/// *, **, *** are all distinct signatures that each retain a full result +/// list, and the per-entry memo is otherwise released only when a new Galaxy deploy publishes +/// a new entry (GalaxyRepository-001). Eviction is FIFO: the memo exists to make paging through +/// ONE filter O(pageSize), and an evicted signature is simply recomputed — the projection is a +/// pure function of the entry and the request, so the answer and its page tokens are unchanged. +/// The factory runs outside the lock; two racing first callers may both compute, and the first +/// to publish wins (same contract as ConcurrentDictionary.GetOrAdd). +/// +/// The memoized projection. +internal sealed class ProjectionMemo + where TValue : class +{ + /// Maximum number of distinct filter signatures retained per cache entry. + public const int Capacity = 64; + + private readonly object _gate = new(); + private readonly Dictionary _items = new(StringComparer.Ordinal); + private readonly Queue _insertionOrder = new(); + + /// Number of signatures currently retained. + public int Count + { + get + { + lock (_gate) + { + return _items.Count; + } + } + } + + /// Returns the memoized value for , computing it when absent. + /// State passed to . + /// The filter signature. + /// Computes the value; must be pure. + /// State for . + public TValue GetOrAdd(string key, Func factory, TState state) + { + lock (_gate) + { + if (_items.TryGetValue(key, out TValue? existing)) + { + return existing; + } + } + + TValue created = factory(key, state); + + lock (_gate) + { + if (_items.TryGetValue(key, out TValue? raced)) + { + return raced; + } + + _items[key] = created; + _insertionOrder.Enqueue(key); + while (_items.Count > Capacity) + { + _items.Remove(_insertionOrder.Dequeue()); + } + + return created; + } + } +} diff --git a/ZB.MOM.WW.GalaxyRepository/tests/ZB.MOM.WW.GalaxyRepository.Tests/ProjectionMemoBoundTests.cs b/ZB.MOM.WW.GalaxyRepository/tests/ZB.MOM.WW.GalaxyRepository.Tests/ProjectionMemoBoundTests.cs new file mode 100644 index 0000000..7fb5e5f --- /dev/null +++ b/ZB.MOM.WW.GalaxyRepository/tests/ZB.MOM.WW.GalaxyRepository.Tests/ProjectionMemoBoundTests.cs @@ -0,0 +1,77 @@ +using ZB.MOM.WW.GalaxyRepository; +using ZB.MOM.WW.GalaxyRepository.Grpc; + +namespace ZB.MOM.WW.GalaxyRepository.Tests; + +/// +/// GalaxyRepository-001 (scadaproj#41): the per-entry filter memo was unbounded, so a client +/// issuing distinct tag_name_glob values (*, **, ...) retained one full +/// result list per glob until the next Galaxy deploy. The memo must stay capped. +/// +public sealed class ProjectionMemoBoundTests +{ + private const int DistinctFilters = 500; + + [Fact] + public void DiscoverHierarchy_memo_is_bounded_under_distinct_client_globs() + { + GalaxyHierarchyCacheEntry entry = CreateEntry(); + + for (int i = 1; i <= DistinctFilters; i++) + { + GalaxyHierarchyProjector.Project(entry, new DiscoverHierarchyRequest { TagNameGlob = new string('*', i) }, null, 0, 1); + } + + Assert.InRange(GalaxyHierarchyProjector.MemoCount(entry), 1, ProjectionMemo.Capacity); + } + + [Fact] + public void BrowseChildren_memo_is_bounded_under_distinct_client_globs() + { + GalaxyHierarchyCacheEntry entry = CreateEntry(); + + for (int i = 1; i <= DistinctFilters; i++) + { + GalaxyBrowseProjector.ProjectChildren(entry, new BrowseChildrenRequest { TagNameGlob = new string('*', i) }, null, 0, 1); + } + + Assert.InRange(GalaxyBrowseProjector.MemoCount(entry), 1, ProjectionMemo.Capacity); + } + + [Fact] + public void Evicted_signature_is_recomputed_with_the_same_answer() + { + GalaxyHierarchyCacheEntry entry = CreateEntry(); + DiscoverHierarchyRequest first = new() { TagNameGlob = "Plant.*" }; + GalaxyHierarchyQueryResult before = GalaxyHierarchyProjector.Project(entry, first, null, 0, 10); + + for (int i = 1; i <= DistinctFilters; i++) + { + GalaxyHierarchyProjector.Project(entry, new DiscoverHierarchyRequest { TagNameGlob = new string('*', i) }, null, 0, 1); + } + + GalaxyHierarchyQueryResult after = GalaxyHierarchyProjector.Project(entry, first, null, 0, 10); + Assert.Equal(before.TotalObjectCount, after.TotalObjectCount); + Assert.Equal(before.FilterSignature, after.FilterSignature); + Assert.Equal(before.Objects.Select(o => o.GobjectId), after.Objects.Select(o => o.GobjectId)); + } + + private static GalaxyHierarchyCacheEntry CreateEntry() + { + GalaxyObject[] objects = + [ + new() { GobjectId = 1, ParentGobjectId = 0, IsArea = true, ContainedName = "Plant", BrowseName = "Plant", TagName = "Plant" }, + new() { GobjectId = 2, ParentGobjectId = 1, ContainedName = "Pump", BrowseName = "Pump", TagName = "Plant.Pump" }, + new() { GobjectId = 3, ParentGobjectId = 1, ContainedName = "Valve", BrowseName = "Valve", TagName = "Plant.Valve" }, + ]; + return GalaxyHierarchyCacheEntry.Empty with + { + Status = GalaxyCacheStatus.Healthy, + Sequence = 1, + LastSuccessAt = DateTimeOffset.UtcNow, + Objects = objects, + Index = GalaxyHierarchyIndex.Build(objects), + ObjectCount = objects.Length, + }; + } +} diff --git a/code-reviews/GalaxyRepository/findings.md b/code-reviews/GalaxyRepository/findings.md index c921ed6..52ffe90 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 | 8 | +| Open findings | 7 | ## Summary @@ -64,7 +64,7 @@ closed when identity is absent, so the empty-means-unscoped contract is handled | Severity | Medium | | Priority | P2 | | Category | Performance & resource management | -| Status | Open | +| 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** @@ -108,7 +108,7 @@ globs and asserts the memo size. **Resolution** -_Unresolved._ +Resolved 2026-11-09 in the commit tagged `(scadaproj#41)` (GalaxyRepository `0.2.1`, unpublished) — new internal `ProjectionMemo` (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 diff --git a/code-reviews/README.md b/code-reviews/README.md index 97d4abd..933a948 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -40,7 +40,7 @@ code-reviews/ ## Summary -10 of 10 libraries reviewed. 46 pending findings across all libraries. +10 of 10 libraries reviewed. 45 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 @@ -50,9 +50,9 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low). |----------|---------------| | Critical | 0 | | High | 0 | -| Medium | 13 | +| Medium | 12 | | Low | 33 | -| **Total** | **46** | +| **Total** | **45** | ## 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/2/6 | 8 | 8 | +| [GalaxyRepository](GalaxyRepository/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/1/6 | 7 | 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 | @@ -84,12 +84,11 @@ _None open._ _None open._ -### Medium (13) +### Medium (12) | ID | Library | Title | |----|---------|-------| | Auth-007 | [Auth](Auth/findings.md) | User-bind mode never checks that the entry whose groups it returns is the account that bound | -| GalaxyRepository-001 | [GalaxyRepository](GalaxyRepository/findings.md) | Filter memo has no size limit: client-chosen filters retain unlimited memory until the next deploy | | GalaxyRepository-002 | [GalaxyRepository](GalaxyRepository/findings.md) | Browse-subtree scoping hides the ancestors of an allowed subtree, so scoped lazy browse from the root is empty | | LocalDb-001 | [LocalDb](LocalDb/findings.md) | Sync endpoint is unauthenticated by default, and `ApiKey` is documented as "expected from the peer" | | LocalDb-002 | [LocalDb](LocalDb/findings.md) | Snapshot streams tables by name and rows by `pk_json` text, so FK children land before parents and are dead-lettered for good |