fix(scripting): route engines through CompiledScriptCache (Core.Scripting-016)
Both VirtualTagEngine.Load and ScriptedAlarmEngine.LoadAsync were calling
ScriptEvaluator.Compile directly, bypassing CompiledScriptCache. The
Core.Scripting-008 collectible-ALC fix wired Dispose only through the cache's
Clear()/Dispose(), so the per-publish accretion the -008 fix was meant to
eliminate was still in effect on the actual production path — the headline
'no more restarts needed' guarantee wasn't delivered.
Resolution:
- VirtualTagEngine + ScriptedAlarmEngine each gained a private
CompiledScriptCache<TContext, TResult> instance.
- Both Load methods now call _compileCache.GetOrCompile(source).
- Publish-replace path: _compileCache.Clear() runs alongside the existing
_tags / _alarms clears so the prior generation's ALCs are disposed
before recompile.
- Engine Dispose now calls _compileCache.Dispose() so shutdown actually
releases the emitted assemblies.
Side-fix in CompiledScriptCache: Dispose() set _disposed=true then called
Clear(), but Clear() had a pre-existing 'if (_disposed) return' guard that
aborted the drain unconditionally — making the Dispose-triggered cleanup a
silent no-op. Removed the disposed-guard on Clear() (clearing an empty/
cleared cache is idempotent).
Side-fix in ScriptedAlarmEngine.Dispose: cleared _alarms AFTER the
Task.WhenAll drain. The drain guarantees no background callback is mid-
flight, so clearing is safe. Previously _alarms was deliberately NOT
cleared on Dispose (per Core.ScriptedAlarms-005), but that left the
AlarmState records holding TimedScriptEvaluator → ScriptEvaluator → delegate
references that rooted the emitted assemblies, defeating the cache's
Dispose work on the engine side.
Regression tests:
- VirtualTagEngineTests.Dispose_unloads_compiled_script_assembly
- ScriptedAlarmEngineTests.Dispose_unloads_compiled_predicate_assembly
Both use WeakReference + bounded GC.Collect() to prove the emitted
assembly is reclaimable after engine.Dispose(). The alarms test had to
be synchronous (not 'async Task<WeakReference>') because async state
machines capture locals as state-struct fields, keeping them alive past
the method's apparent end and defeating GC.
Verification:
- Core.Scripting.Tests: 104/104 (unchanged).
- VirtualTags.Tests: 57/57 (was 56 — +1 unload test).
- ScriptedAlarms.Tests: 67/67 (was 66 — +1 unload test).
- All other consumer suites still green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -7,7 +7,7 @@
|
||||
| Review date | 2026-05-23 |
|
||||
| Commit reviewed | `a9be809` |
|
||||
| Status | Reviewed |
|
||||
| Open findings | 5 |
|
||||
| Open findings | 4 |
|
||||
|
||||
## Checklist coverage
|
||||
|
||||
@@ -612,7 +612,7 @@ so the bug surfaces only as a misleading Roslyn diagnostic).
|
||||
| Severity | Medium |
|
||||
| Category | Performance & resource management |
|
||||
| Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs:74-117`, `src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs:139-182` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** The Core.Scripting-008 resolution introduced
|
||||
`ScriptEvaluator.IDisposable` + `CompiledScriptCache.Clear()` that disposes
|
||||
@@ -657,4 +657,41 @@ for each engine: snapshot the per-evaluator emitted assembly via
|
||||
`WeakReference`, call `Load(...)` with a different definition set, and assert
|
||||
the prior generation's assemblies become collectable.
|
||||
|
||||
**Resolution:** _(empty until closed; on close, record the fixing commit SHA, the date, and a one-line description of the fix)_
|
||||
**Resolution:** Resolved 2026-05-23 — took the cleaner route from the
|
||||
recommendation: routed both engines' compile paths through
|
||||
`CompiledScriptCache<TContext, TResult>`. `VirtualTagEngine` and
|
||||
`ScriptedAlarmEngine` each gained a private `_compileCache` instance field,
|
||||
their `Load`/`LoadAsync` methods now call `_compileCache.GetOrCompile(source)`
|
||||
instead of `ScriptEvaluator.Compile(source)` directly, and the cache is cleared
|
||||
on publish-replace alongside the existing `_tags` / `_alarms` clears so the
|
||||
prior generation's ALCs are disposed before recompile. Engine `Dispose` now
|
||||
also calls `_compileCache.Dispose()` so the engine-shutdown path actually
|
||||
releases the emitted assemblies. **Side-fix:** discovered + fixed an
|
||||
adjacent bug in `CompiledScriptCache.Dispose()` itself — it set
|
||||
`_disposed = true` before calling `Clear()`, but `Clear()`'s pre-existing
|
||||
`if (_disposed) return` guard then aborted the drain unconditionally, so
|
||||
the Dispose-triggered cleanup was a silent no-op. Removed the disposed-guard
|
||||
on `Clear()` (the operation is idempotent — clearing an empty/cleared cache
|
||||
is safe). Without this side-fix the engine-Dispose path would have left
|
||||
the cached evaluators rooted forever even though the call chain looked
|
||||
correct. **Side-fix for ScriptedAlarmEngine.Dispose:** moved the pre-existing
|
||||
"do NOT clear `_alarms` here" comment to "clear `_alarms` AFTER the drain"
|
||||
because the AlarmState records hold the `TimedScriptEvaluator`/`ScriptEvaluator`
|
||||
delegates that root the emitted assembly — leaving them in `_alarms` after
|
||||
Dispose was the same root-the-script-forever pattern this finding is about,
|
||||
just on the engine side rather than the cache side. The `_alarms` clear is
|
||||
safe after the `Task.WhenAll` drain because that drain guarantees no
|
||||
background callback is mid-flight. Regression tests added:
|
||||
`VirtualTagEngineTests.Dispose_unloads_compiled_script_assembly` and
|
||||
`ScriptedAlarmEngineTests.Dispose_unloads_compiled_predicate_assembly` —
|
||||
each uses `WeakReference` + bounded `GC.Collect()` to prove the emitted
|
||||
assembly is reclaimable after `engine.Dispose()`. **Important test pattern
|
||||
detail:** the alarms test originally failed because its helper was
|
||||
`async Task<WeakReference>` — async state machines capture locals as
|
||||
state-struct fields and can keep them alive past the method's apparent end.
|
||||
Rewrote as a synchronous helper using `LoadAsync(...).GetAwaiter().GetResult()`
|
||||
inside two cooperating `[MethodImpl(MethodImplOptions.NoInlining)]` helpers
|
||||
(`CompileAlarmAndCaptureWeak` + `ExtractEmittedAssemblyWeakRef`) so the
|
||||
intermediate reflection locals die when each helper returns. Test totals
|
||||
after fix: Core.Scripting 104 green (unchanged); VirtualTags 57 green (was
|
||||
56 — +1 unload test); ScriptedAlarms 67 green (was 66 — +1 unload test).
|
||||
|
||||
Reference in New Issue
Block a user