refactor(siteruntime): scope SiteScriptCompileCache to the ScriptCompilationService singleton — close register row 100
The compile cache was a `static` class, so its entry table and its Hits/Count
counters were one process-global blob every test class in the SiteRuntime
assembly wrote into. Row 91 last wave neutered the resulting flakes (per-test
GUID keys + one-sided counter bounds), but the shared mutable state itself
remained: any test calling Clear() evicted entries other parallel classes had
just inserted, and a Clear() from a class outside the serialized collection
would degrade the others to recompiles and re-open the row-91 flake class.
PREMISE CORRECTION: the register speculated production clears "per-instance or
per-deployment script entries" and offered ClearKeys(prefix) as the natural fit.
There are NO production callers of Clear() at all — grep of src/ finds only
GetOrAdd (ScriptCompilationService.CompileCore). Clear() existed solely so
tests could reset the global. A key-scoped API would therefore have been new
production surface built for nobody, so the instance-scoped seam is the fix and
Clear() is deleted outright rather than migrated.
- SiteScriptCompileCache: `internal static class` -> `internal sealed class`;
Cache/EvictionLock/_hits/_accessSequence become instance fields. LRU batch
eviction, the double-checked sweep and the ConcurrentDictionary.ToArray()
snapshot are byte-for-byte the same policy. Clear() removed.
- ScriptCompilationService owns one cache. Public ctor (the DI path) builds its
own; a new internal ctor takes one for tests. Production N4 sharing is
unchanged and now rests on the DI SINGLETON registration — the one service
instance is threaded to DeployCompileValidator and to every Instance Actor —
so that lifetime is load-bearing and is pinned by a new registration test.
- Tests: all three Clear() callers migrated off it. Each test now owns its cache,
which lets the assertions get STRONGER — the row-91 workarounds are retired:
Hits/Count are exact again (no >= baseline bounds), the per-test GUID key
token is gone, and ForeignEntrySlack (512 entries of allowance on the bound)
drops to the racing-thread count in the concurrency test. New test
SeparateCaches_DoNotShareEntriesOrCounters pins the scoping directly: filling
one cache past MaxEntries leaves another's entry, hit count and identity
intact. DeploymentWarmThenGateTests additionally asserts Count == 1 now that
its counter is private.
- The [Collection("SiteScriptCompileCache")] serialization from row 91 is
RELAXED (removed from all three classes) — they no longer touch any shared
state, so there is nothing left to serialize.
- Docs: Component-SiteRuntime.md gains the ownership/singleton note and drops
the Clear() reference; DeploymentManagerActor / DeployCompileValidator /
ServiceCollectionExtensions comments corrected to say where the cache lives.
Build: 0 warnings. SiteRuntime.Tests 643/643, three consecutive full-parallel
runs. AuditLog.Tests correlation classes (other ScriptCompilationService
consumers) green.
This commit is contained in:
+20
-13
@@ -29,13 +29,17 @@ namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.Actors;
|
||||
/// answer its deployer instead of leaving it to Ask-timeout, and commands for DIFFERENT
|
||||
/// instances must not block each other.</para>
|
||||
///
|
||||
/// <para>Shares the <c>SiteScriptCompileCache</c> collection because the batch pre-warm test
|
||||
/// asserts on that process-wide cache's hit counter.</para>
|
||||
/// <para>The batch pre-warm test asserts on the compile cache's hit counter. As of register
|
||||
/// row 100 that cache is owned by the <c>ScriptCompilationService</c> instance this fixture
|
||||
/// builds — the one it threads into the Deployment Manager and, through it, into every Instance
|
||||
/// Actor — so the counter is private to the test. The old
|
||||
/// <c>[Collection("SiteScriptCompileCache")]</c> serialisation against the two
|
||||
/// <c>Scripts/</c> test classes is therefore gone: nothing here is shared any more.</para>
|
||||
/// </summary>
|
||||
[Collection("SiteScriptCompileCache")]
|
||||
public class DeploymentWarmThenGateTests : TestKit, IDisposable
|
||||
{
|
||||
private readonly SiteStorageService _storage;
|
||||
private readonly SiteScriptCompileCache _compileCache;
|
||||
private readonly ScriptCompilationService _compilationService;
|
||||
private readonly SharedScriptLibrary _sharedScriptLibrary;
|
||||
private readonly TestLocalDb _localDb;
|
||||
@@ -45,8 +49,9 @@ public class DeploymentWarmThenGateTests : TestKit, IDisposable
|
||||
_localDb = TestLocalDb.CreateTemp("dm-warm-gate-test");
|
||||
_storage = new SiteStorageService(_localDb.Db, NullLogger<SiteStorageService>.Instance);
|
||||
_storage.InitializeAsync().GetAwaiter().GetResult();
|
||||
_compileCache = new SiteScriptCompileCache();
|
||||
_compilationService = new ScriptCompilationService(
|
||||
NullLogger<ScriptCompilationService>.Instance);
|
||||
NullLogger<ScriptCompilationService>.Instance, _compileCache);
|
||||
_sharedScriptLibrary = new SharedScriptLibrary(
|
||||
_compilationService, NullLogger<SharedScriptLibrary>.Instance);
|
||||
}
|
||||
@@ -247,21 +252,23 @@ public class DeploymentWarmThenGateTests : TestKit, IDisposable
|
||||
await _storage.StoreDeployedConfigAsync(
|
||||
"BatchTwo", ConfigJson("BatchTwo", sharedCode), "d2", "h2", true);
|
||||
|
||||
SiteScriptCompileCache.Clear();
|
||||
// Baseline rather than Assert.Equal(0, Hits): Hits is a process-global counter and test
|
||||
// classes outside this collection compile scripts in parallel, so a foreign hit can land
|
||||
// between the Clear() and the read (deferred-work register row 91). Everything below is
|
||||
// asserted as a delta over this baseline.
|
||||
var hitsBefore = SiteScriptCompileCache.Hits;
|
||||
// No Clear() and no baseline subtraction: this cache is the fixture's own and nothing
|
||||
// has compiled into it yet (register rows 91/100 — the counter used to be a
|
||||
// process-global that other test classes bumped in parallel, forcing a delta-over-
|
||||
// baseline reading). Only the Deployment Manager below writes to it.
|
||||
Assert.Equal(0, _compileCache.Hits);
|
||||
|
||||
CreateDeploymentManager();
|
||||
|
||||
AwaitAssert(() =>
|
||||
{
|
||||
// One compile, then repeated hits: the second pre-warm plus both PreStarts.
|
||||
Assert.True(SiteScriptCompileCache.Hits - hitsBefore >= 3,
|
||||
$"expected the pre-warmed body to be served from cache, saw " +
|
||||
$"{SiteScriptCompileCache.Hits - hitsBefore} hits over baseline {hitsBefore}");
|
||||
Assert.True(_compileCache.Hits >= 3,
|
||||
$"expected the pre-warmed body to be served from cache, saw {_compileCache.Hits} hits");
|
||||
}, TimeSpan.FromSeconds(20));
|
||||
|
||||
// …and exactly one entry for the one shared body: the hits above were the SAME compiled
|
||||
// script being reused, not several bodies each compiled once.
|
||||
Assert.Equal(1, _compileCache.Count);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user