Files
ScadaBridge/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ScriptCompileVerdictCacheEvictionTests.cs
T
Joseph Doherty e0e4b24679 fix(deploy+cli): review findings — honest CLI timeouts, watermark-complete staleness, phase-2 staging, lock-safe cancellation
Six adversarial-review findings, each verified against the code first.

F1 (HIGH) CLI HttpClient capped every call at min(30s, caller timeout),
silently truncating deploy site's 5-minute BulkDeployTimeout and the
5-minute bundle export/preview/import calls — which printed a fake
"504 Request timed out" while the server kept working. HttpClient.Timeout
is now Timeout.InfiniteTimeSpan (the per-call CTS is the single overall
deadline, connect included) with the connect phase bounded separately on
SocketsHttpHandler.ConnectTimeout. The env override is renamed to
SCADABRIDGE_HTTP_CONNECT_TIMEOUT_SECONDS to match its new meaning.

F2 (HIGH) StaleInstanceProbe's process-static memo served stale hashes
because nothing bumped the watermark on three paths:
  (a) BundleImporter commits through the raw DbContext, so no import ever
      moved the watermark — a second import overwriting the same template
      could be OMITTED from ImportResult.StaleInstanceIds. It now bumps
      once per apply ATTEMPT: after the commit, and after the rollback too
      (the probe runs pre-commit, so a rolled-back attempt leaves memos for
      state that never landed; bumping on both paths is the simplest
      correct shape, versus threading transaction awareness through a
      process-static cache).
  (b) CollectWatermarkBumps' default: arm silently no-op'd, contradicting
      its own doc. It now sets unattributed=true — an over-broad bump costs
      extra work, a missed one produces stale work.
  (c) DataConnection edits route through SiteRepository.SaveChangesAsync,
      which had no watermark at all, yet Protocol/Primary+Backup config/
      FailoverRetryCount are revision-hash inputs. It now bumps (BumpAll —
      a connection has no owning template) after a commit that touched one.

F3 (MED) CLI TemplateTableProjection read child ARRAYS, but ListTemplates
now returns database-projected TemplateSummary rows, so template list
printed all zeros. It now prefers the *Count scalars and falls back to
array length (template get still returns full entities). --detail help
text and README corrected: a listing cannot yield definitions, so --detail
renders the raw summary payload and template get --id is the full dump.

F4 (MED) DeploySiteAsync staged every PendingDeployment in phase 1 against
a 5-min TTL while phase 2 reached them one batch at a time, so tail
instances' fetch tokens could expire before their command was sent.
Staging moved into phase 2, immediately before each send; prepare keeps
its flatten/validate/record work. The staging write is the phase's only
repository touch and is serialised behind a 1-permit semaphore, so the
non-thread-safe DbContext constraint holds and the sends stay concurrent.

F5 (MED, latent) DeploySiteAsync leaked every held operation lock if
cancelled — a wedged per-instance semaphore is permanent for the process.
Phase 2 no longer throws (cancellation is recorded as a per-instance
outcome so phase 3 still runs), and an escape from phase 1 or 3 now
unwinds every unfinalised entry: Failed status + lock release.

F6 (LOW) ScriptCompileVerdictCache's promotion wrote hot directly,
bypassing SegmentCapacity (true ceiling 3x against a documented 2x).
Promotion now goes through Store, keeping generational semantics; _hot
and _cold are volatile.

Tests: CLI 396, DeploymentManager 133, ManagementService 494,
TemplateEngine 478, ScriptAnalysis 60, Transport 157, Transport
integration 106, ConfigurationDatabase 366 — all green, 0 build warnings.
The F2/F4/F5 regression tests were each confirmed to FAIL with their fix
reverted.
2026-08-14 23:51:04 -04:00

166 lines
6.4 KiB
C#
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
using ZB.MOM.WW.ScadaBridge.TemplateEngine.Validation;
namespace ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests.Validation;
/// <summary>
/// WP2.5: the verdict cache's eviction policy. Overflow used to <c>Clear()</c> the
/// whole cache, which re-opened the non-collectible <c>InteractiveAssemblyLoader</c>
/// leak the cache exists to bound — every hot script had to be recompiled, and every
/// recompile loads another assembly that can never be unloaded. These tests pin the
/// replacement: eviction is segmented, so entries in active use survive it.
///
/// <para>
/// Serialised with the other verdict-cache tests: the cache is process-wide static
/// state, so two test classes filling it concurrently would see each other's
/// entries.
/// </para>
/// </summary>
[Collection("ScriptCompileVerdictCache")]
public class ScriptCompileVerdictCacheEvictionTests
{
private const string Surface = "TestSurface";
/// <summary>Entries needed to force at least one generation rotation.</summary>
private const int OverflowCount = 5000;
private static (bool Ok, string? Error) Lookup(string code, Func<(bool, string?)> factory) =>
ScriptCompileVerdictCache.GetOrAdd(Surface, code, factory);
[Fact]
public void Overflow_KeepsHotEntry_AndDoesNotClearEverything()
{
ScriptCompileVerdictCache.Clear();
const string hotCode = "// the script every deploy re-validates";
var hotCompiles = 0;
(bool, string?) HotFactory()
{
hotCompiles++;
return (true, null);
}
Lookup(hotCode, HotFactory);
Assert.Equal(1, hotCompiles);
// Push far more distinct scripts through than the cache can hold, touching
// the hot entry as we go — which is exactly what a real workload does, and
// exactly what wholesale Clear() used to throw away.
for (var i = 0; i < OverflowCount; i++)
{
Lookup($"// filler {i}", static () => (true, null));
if (i % 25 == 0)
Lookup(hotCode, HotFactory);
}
// At least one rotation happened...
Assert.True(ScriptCompileVerdictCache.Evictions > 0,
"the overflow did not trigger a single eviction — the test no longer exercises the policy");
// ...and the hot entry was never recompiled, because a hit in the cold
// generation promotes it back into hot rather than letting it age out.
Assert.Equal(1, hotCompiles);
// A final read still hits.
var compilesBefore = hotCompiles;
Lookup(hotCode, HotFactory);
Assert.Equal(compilesBefore, hotCompiles);
}
[Fact]
public void Overflow_RetainsRecentEntries_RatherThanDroppingAll()
{
ScriptCompileVerdictCache.Clear();
for (var i = 0; i < OverflowCount; i++)
Lookup($"// bulk {i}", static () => (true, null));
Assert.True(ScriptCompileVerdictCache.Evictions > 0);
// The most recently inserted entry is in the hot generation, so it must
// still be cached. Under the old Clear()-on-overflow policy the cache could
// be left holding a single entry after a rotation.
var recompiled = false;
Lookup($"// bulk {OverflowCount - 1}", () =>
{
recompiled = true;
return (true, null);
});
Assert.False(recompiled, "the most recent entry was evicted; eviction is not retaining the hot generation");
Assert.True(ScriptCompileVerdictCache.Count > 1,
$"cache retained only {ScriptCompileVerdictCache.Count} entries after eviction");
}
[Fact]
public void Overflow_KeepsCacheBounded()
{
ScriptCompileVerdictCache.Clear();
for (var i = 0; i < OverflowCount; i++)
Lookup($"// bounded {i}", static () => (true, null));
// Two generations of 2048 — the same 4096 ceiling the previous policy had,
// now reached by demotion rather than by dropping everything.
Assert.True(ScriptCompileVerdictCache.Count <= 4096,
$"cache grew to {ScriptCompileVerdictCache.Count} entries, exceeding its two-generation bound");
}
/// <summary>
/// The bound must hold under a PROMOTION-heavy workload too, not just a
/// pure-insert one. Promotion used to write straight into hot
/// (<c>_hot[key] = verdict</c>) with no capacity check, so re-reading a working
/// set bigger than one segment pulled the whole cold generation back into hot
/// on top of what hot already held — hot alone reached 2 × SegmentCapacity and
/// the total 3 ×, against a documented 2 ×. Pure-insert overflow never hits
/// that path because every key is distinct.
/// </summary>
[Fact]
public void PromotionOverflow_KeepsCacheBounded()
{
ScriptCompileVerdictCache.Clear();
// Fill past one rotation so a large working set is sitting in cold with
// hot already partly full.
const int WorkingSet = 3000;
for (var i = 0; i < WorkingSet; i++)
Lookup($"// promo {i}", static () => (true, null));
Assert.True(ScriptCompileVerdictCache.Evictions > 0);
// Re-read the whole working set: every entry still in cold promotes.
for (var i = 0; i < WorkingSet; i++)
Lookup($"// promo {i}", static () => (true, null));
Assert.True(ScriptCompileVerdictCache.Count <= 4096,
$"cache grew to {ScriptCompileVerdictCache.Count} entries under promotion, exceeding its two-generation bound");
}
[Fact]
public void SurfaceIsPartOfTheKey_AcrossEviction()
{
ScriptCompileVerdictCache.Clear();
const string code = "// same body, two surfaces";
Lookup(code, static () => (true, null));
var otherSurfaceCompiled = false;
var verdict = ScriptCompileVerdictCache.GetOrAdd("OtherSurface", code, () =>
{
otherSurfaceCompiled = true;
return (false, "not valid against this surface");
});
// A verdict is never interchangeable across globals surfaces, and the
// segmented cache must not weaken that.
Assert.True(otherSurfaceCompiled);
Assert.False(verdict.Ok);
}
}
/// <summary>
/// Serialises every test that touches the process-wide
/// <see cref="ScriptCompileVerdictCache"/> static.
/// </summary>
[CollectionDefinition("ScriptCompileVerdictCache")]
public class ScriptCompileVerdictCacheCollection;