Core.ScriptedAlarms-009 resolution: replace the per-call Dictionary +
AlarmPredicateContext allocation with a per-alarm reusable AlarmScratch
held in _scratchByAlarmId, refilled in place under _evalGate on each
evaluation. The hot path no longer allocates per upstream tag change.
Why this matters:
On a busy line where many tags feeding many alarms change frequently,
the old BuildReadCache allocated a fresh dictionary + context on every
predicate evaluation — a steady stream of short-lived allocations the
GC eventually has to reclaim. With the reuse, the dictionary and
context are allocated once per alarm (on first evaluation) and refilled
in place across every subsequent re-eval.
Implementation:
- New private AlarmScratch class holds the reusable
Dictionary<string, DataValueSnapshot> read cache (pre-sized to the
alarm's Inputs.Count) and the AlarmPredicateContext that wraps it by
reference. The context observes refilled values without being
re-created.
- ConcurrentDictionary<string, AlarmScratch> _scratchByAlarmId on the
engine, cleared in LoadAsync alongside _alarms so a config-publish
drops the prior generation's scratch (Inputs / Logger may change).
- EvaluatePredicateToStateAsync looks up scratch via GetOrAdd, calls
the new RefillReadCache(Dictionary, IReadOnlySet) helper to clear +
repopulate the dictionary in place, then runs the predicate against
the reused context.
- BuildReadCache removed.
Safety:
Reuse is serialised under _evalGate which guarantees no two threads
ever observe the same scratch in a half-refilled state. The
AlarmPredicateContext is bound to the scratch dictionary by reference,
so the predicate's ctx.GetTag(path) sees the freshly-refilled values
rather than a stale snapshot.
Verification:
- All 66 ScriptedAlarms tests pass (was 63 — three new regression tests
locking the reuse contract).
- All 56 VirtualTags tests still pass (unchanged).
- All 104 Core.Scripting tests still pass (unchanged).
New tests in ScriptedAlarmEngineTests:
- Reevaluation_reuses_the_same_read_cache_dictionary — asserts
ReferenceEquals(scratch_before, scratch_after) across two
evaluations of the same alarm.
- Reevaluation_reuses_the_same_predicate_context — same, for the
context.
- LoadAsync_drops_the_prior_generations_scratch — asserts a config
publish wipes the prior scratch (so a stale Logger / Inputs can't
leak into the new generation).
Internal test hooks TryGetScratchReadCacheForTest /
TryGetScratchContextForTest added via the existing
InternalsVisibleTo for the tests project. Kept internal — not part of
the public engine surface.
Docs:
- docs/v2/Galaxy.Performance.md "Scripted-alarm engine" section
rewritten as "hot-path allocation reuse" documenting the new
contract + reuse safety reasoning + the three regression tests.
- code-reviews/Core.ScriptedAlarms/findings.md -009 flipped
Won't Fix → Resolved.
- code-reviews/README.md regenerated.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Core.Scripting-008 resolution: replace the legacy CSharpScript.CreateDelegate
path with hand-rolled CSharpCompilation + Emit + collectible AssemblyLoadContext,
so per-publish compile accretion no longer requires a server restart to reclaim.
Why this was needed:
Roslyn's CSharpScript path emits dynamically-compiled script assemblies into
the default AssemblyLoadContext, which is non-collectible. Across config-
publish generations each Clear() drops dictionary entries but the emitted
assemblies stay loaded for process lifetime, so memory grows steadily on
long-running servers with frequent publishes. The accepted-limitation note
in docs/VirtualTags.md recommended scheduled restarts as the workaround;
operator feedback was that restarts are difficult, so the underlying
limitation was the right thing to fix.
Implementation:
- New ScriptAssemblyLoadContext(name, isCollectible: true) hosts one emitted
script assembly per evaluator.
- ScriptEvaluator.Compile synthesises a wrapper class around the user source
(CompiledScript.Run(globals) — explicit return required per ordinary C#
semantics, which every existing script already uses), builds a
CSharpCompilation against the sandbox references, runs the
ForbiddenTypeAnalyzer over the semantic model unchanged, emits to an
in-memory PE stream, loads via ScriptAssemblyLoadContext.LoadFromStream,
and binds a strongly-typed Func<ScriptGlobals<TContext>, TResult> delegate
via reflection.
- ScriptEvaluator now implements IDisposable — Dispose calls
AssemblyLoadContext.Unload(), which makes the emitted assembly eligible
for GC at the next collection cycle.
- CompiledScriptCache.Clear() disposes every materialised evaluator before
dropping its dictionary entry; CompiledScriptCache itself is now
IDisposable for graceful server shutdown.
- ScriptSandbox.Build returns a new SandboxConfig (References + Imports)
instead of a Roslyn ScriptOptions; references now span BCL via the
TRUSTED_PLATFORM_ASSEMBLIES set filtered to System.* + netstandard +
Microsoft.Win32.Registry, so forbidden BCL types resolve at compile and
ForbiddenTypeAnalyzer is the sole security gate (consistent with the
Core.Scripting-001 / -002 model — references-list-only restriction is
porous against type forwarding, so the analyzer must be the real gate).
Verification:
- All 104 Core.Scripting tests pass (was 101 — three new regression tests
locking the unload contract).
- All 56 VirtualTags tests pass (unchanged).
- All 63 ScriptedAlarms tests pass (unchanged).
- New CompiledScriptCacheTests:
- Dispose_unloads_compiled_script_assembly_load_context — proves single-
evaluator ALC unload via WeakReference + bounded GC.Collect() loop.
- Clear_disposes_every_materialised_evaluator — proves publish-replace
releases every prior generation's ALC.
- GetOrCompile_after_Dispose_throws_ObjectDisposedException — locks the
post-dispose contract.
Docs:
- docs/VirtualTags.md "Compile cache" section rewritten: the accepted-
limitation note replaced with the unload contract + the new authoring
convention (explicit return).
- docs/ScriptedAlarms.md cross-reference updated to drop the obsolete
restart guidance.
- code-reviews/Core.Scripting/findings.md Core.Scripting-008 flipped
Won't Fix → Resolved with the implementation summary.
- code-reviews/README.md regenerated.
Pre-existing breakage note: Driver.Galaxy fails the solution-wide build on
master because its ProjectReference to the sibling mxaccessgw repo's
MxGateway.Client targets a path that the sibling repo no longer has after a
recent restructuring. This is unrelated to Core.Scripting-008 and was
verified to exist on master before this branch was cut.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Driver.FOCAS.Cli-005 follow-up: extend the SnapshotFormatter.FormatStatus
shortlist with the five Bad* codes the native-protocol mappers (FOCAS,
AbCip, AbLegacy) emit but which the shortlist previously left unnamed,
so they rendered only as severity-class 'Bad' instead of the documented
'BadDeviceFailure' / 'BadNotWritable' / ... names operators are told to
read off probe/write output.
Added entries:
0x80020000 BadInternalError
0x803B0000 BadNotWritable
0x803C0000 BadOutOfRange
0x803D0000 BadNotSupported
0x80550000 BadDeviceFailure
(BadTimeout 0x800A0000 was already added under Driver.Cli.Common-001.)
Tests: SnapshotFormatterTests gains a new [Theory]
FormatStatus_names_native_driver_emitted_codes covering the five names,
and the existing well-known [Theory] is extended with the same entries
to enforce exact '0x... (Name)' rendering. Suite now 47 green (was 42).
Flips Driver.FOCAS.Cli-005 from Deferred to Resolved; README regenerated.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Batch 7 closed the last Open findings in Client.UI. The review backlog
is now empty: 0 Open findings across all 31 modules.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Batch 6 cleared Open findings in Driver.FOCAS.Cli (1 deferred to
Driver.Cli.Common), Driver.Cli.Common, Driver.Historian.Wonderware.Client,
Client.CLI, and Client.Shared.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Batch 5 cleared Open findings in Driver.AbCip.Cli, Driver.AbLegacy.Cli,
Driver.S7.Cli, Driver.TwinCAT.Cli, and Driver.Modbus.Cli.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Batch 4 cleared Open findings in Driver.TwinCAT, Driver.Modbus,
Driver.OpcUaClient, Driver.Historian.Wonderware, and Driver.Modbus.Addressing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Batch 3 cleared Open findings in Driver.Galaxy, Driver.AbCip,
Driver.AbLegacy, Driver.FOCAS, and Driver.S7.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Resolution prose was already recorded under Core.Scripting commit
(0454822); status was left as Open. Flip to Won't Fix to match.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Batch 2 cleared Open findings in Core.ScriptedAlarms, Core.Scripting,
Core.VirtualTags, Admin, and Server (Core.ScriptedAlarms-009 documented
under Won't Fix per the recommendation).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Batch 1 cleared Open findings in Core, Core.Abstractions, Core.AlarmHistorian,
Configuration, and Analyzers.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Records the post-review finding discovered during browser smoke-testing: the
Admin-003 hub hardening was incomplete — the server-side Blazor HubConnection
clients had no way to authenticate, so hub negotiate 401'd and four cluster
pages threw unhandled 500s. Logged as Admin-013 (High, Error handling &
resilience), Status Resolved, fixed by commits f254539 + 8d5dbb4.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
All Medium-severity code-review findings across the 29 reviewed modules
are now Resolved. The Pending findings table holds only Low-severity items.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewed all 31 src/ production projects against the 10-category
checklist in REVIEW-PROCESS.md. Each module gets its own findings.md;
code-reviews/README.md is regenerated from them.
334 findings: 6 Critical, 46 High, 126 Medium, 156 Low.
Critical findings:
- Server-001: WriteNodeIdUnknown recurses unconditionally — a HistoryRead
on an unresolvable node crashes the process (remote DoS).
- Admin-001/002: app-wide auth bypass (RouteView not AuthorizeRouteView)
plus unauthenticated mutating routes.
- Core.Scripting-001: System.Environment reachable from operator scripts;
Environment.Exit() terminates the server.
- Core.AlarmHistorian-001: rowIds/events parallel-list desync on a corrupt
payload misapplies outcomes — silent alarm-event data loss.
- Driver.Galaxy-001: ReconnectSupervisor is built but never triggered, so
a transient gateway drop permanently kills the event stream.
All findings are Status=Open; resolution is tracked per REVIEW-PROCESS.md
section 4. Review only — no source code changed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adapts the code-review procedure, folder layout, template, and tooling
from the sibling mxaccessgw repo to lmxopcua.
- REVIEW-PROCESS.md: per-module review workflow — a module is one src/
or tests/ project (ZB.MOM.WW.OtOpcUa. prefix stripped); 10-category
checklist; finding IDs/severities/statuses; re-review rules.
- code-reviews/_template/findings.md: per-module findings template.
- code-reviews/regen-readme.py: generates the cross-module README.md
index from the per-module findings.md files; --check gates staleness
and consistency.
- code-reviews/test_regen_readme.py: dependency-free generator tests.
- code-reviews/prompt.md: orchestration prompt for clearing the backlog.
- code-reviews/README.md: generated index (no modules reviewed yet).
- scripts/check-code-reviews-readme.ps1: CI / pre-commit check wrapper.
Adapted to this repo: ZB.MOM.WW.OtOpcUa module naming, OtOpcUa
conventions checklist (in-process GalaxyDriver + mxaccessgw,
contained-name vs tag-name, ACL at DriverNodeManager), single .NET
solution build/test commands, and the lmxopcua design docs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
