From 702de910adc6e8ca1a07df1623eef282cfbe8ee2 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 12 Aug 2026 16:42:27 -0400 Subject: [PATCH] test(scripts): pin null-result and definition-agnostic caching; review polish --- .../CachingScriptMetadataResolver.cs | 18 ++++- .../CachingScriptMetadataResolverTests.cs | 67 +++++++++++++++++++ 2 files changed, 82 insertions(+), 3 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/CachingScriptMetadataResolver.cs b/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/CachingScriptMetadataResolver.cs index 0ee74998..3c833974 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/CachingScriptMetadataResolver.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/CachingScriptMetadataResolver.cs @@ -56,13 +56,25 @@ public sealed class CachingScriptMetadataResolver : MetadataReferenceResolver private readonly MetadataReferenceResolver _inner; + // Keyed on the assembly identity display name, compared case-insensitively to + // match .NET assembly-name binding, which treats simple names case-insensitively. + // A null VALUE is cached too: "not found" is a legitimate, sticky result. private readonly ConcurrentDictionary _missingByIdentity = new(StringComparer.OrdinalIgnoreCase); + // Deliberately case-SENSITIVE (the tuple's default comparer): baseFilePath is a + // filesystem path, and on the Linux containers these nodes run in, two paths + // differing only in case are two different files. private readonly ConcurrentDictionary<(string Reference, string? BaseFilePath, MetadataReferenceProperties Properties), - ImmutableArray> _referencesByPath = new(); + ImmutableArray> _referencesByArgs = new(); - /// Creates a decorator over the given inner resolver. Exposed for tests; production uses . + /// + /// Creates a decorator over the given inner resolver. Exposed for tests; + /// production code must NEVER call this constructor directly — always attach + /// . The cache lives on the instance, so a second + /// instance has a second (empty) cache and silently reintroduces the + /// per-compile native-metadata leak on whichever surface uses it. + /// /// The resolver whose results are memoized. public CachingScriptMetadataResolver(MetadataReferenceResolver inner) => _inner = inner; @@ -79,7 +91,7 @@ public sealed class CachingScriptMetadataResolver : MetadataReferenceResolver /// public override ImmutableArray ResolveReference( string reference, string? baseFilePath, MetadataReferenceProperties properties) - => _referencesByPath.GetOrAdd( + => _referencesByArgs.GetOrAdd( (reference, baseFilePath, properties), _ => _inner.ResolveReference(reference, baseFilePath, properties)); diff --git a/tests/ZB.MOM.WW.ScadaBridge.ScriptAnalysis.Tests/CachingScriptMetadataResolverTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ScriptAnalysis.Tests/CachingScriptMetadataResolverTests.cs index 59044fea..2cfc12ce 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ScriptAnalysis.Tests/CachingScriptMetadataResolverTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ScriptAnalysis.Tests/CachingScriptMetadataResolverTests.cs @@ -37,6 +37,27 @@ public class CachingScriptMetadataResolverTests public override int GetHashCode() => 0; } + /// Inner resolver that finds nothing — models an assembly absent from every search path. + private sealed class NullReturningResolver : MetadataReferenceResolver + { + public int MissingCalls; + public override bool ResolveMissingAssemblies => true; + + public override PortableExecutableReference? ResolveMissingAssembly( + MetadataReference definition, AssemblyIdentity referenceIdentity) + { + Interlocked.Increment(ref MissingCalls); + return null; + } + + public override ImmutableArray ResolveReference( + string reference, string? baseFilePath, MetadataReferenceProperties properties) + => []; + + public override bool Equals(object? other) => ReferenceEquals(this, other); + public override int GetHashCode() => 0; + } + private static readonly AssemblyIdentity SomeIdentity = new("System.Fake", new Version(1, 0, 0, 0)); private static readonly MetadataReference SomeDefinition = MetadataReference.CreateFromFile(typeof(object).Assembly.Location); @@ -66,6 +87,52 @@ public class CachingScriptMetadataResolverTests Assert.Equal(2, inner.MissingCalls); } + /// + /// A not-found result is cached and sticky BY DESIGN: the inner resolver + /// searches a fixed set of directories that does not change over a node's + /// lifetime, so re-asking for an assembly that was not there only pays the + /// probe cost again. Pinned because caching nulls is the kind of thing a + /// future reader "fixes" without realising it is deliberate. + /// + [Fact] + public void ResolveMissingAssembly_NotFound_CachesTheNullResult() + { + var inner = new NullReturningResolver(); + var sut = new CachingScriptMetadataResolver(inner); + + var first = sut.ResolveMissingAssembly(SomeDefinition, SomeIdentity); + var second = sut.ResolveMissingAssembly(SomeDefinition, SomeIdentity); + + Assert.Equal(1, inner.MissingCalls); + Assert.Null(first); + Assert.Null(second); + } + + /// + /// Locks in the documented cache-key contract: the key is the assembly + /// identity ALONE, and the requesting definition (whose directory is a + /// search path in the inner resolver) is deliberately ignored, because every + /// ScadaBridge node resolves from one publish directory plus the shared + /// framework. If that assumption ever stops holding, this test fails and the + /// widening becomes an intentional diff rather than a silent one. + /// + [Fact] + public void ResolveMissingAssembly_SameIdentityDifferentDefinitions_ResolvesOnce() + { + var otherDefinition = MetadataReference.CreateFromFile(typeof(Enumerable).Assembly.Location); + Assert.NotEqual( + typeof(object).Assembly.Location, + typeof(Enumerable).Assembly.Location); // guard: the two definitions really are distinct files + + var inner = new CountingResolver(); + var sut = new CachingScriptMetadataResolver(inner); + + sut.ResolveMissingAssembly(SomeDefinition, SomeIdentity); + sut.ResolveMissingAssembly(otherDefinition, SomeIdentity); + + Assert.Equal(1, inner.MissingCalls); + } + [Fact] public void ResolveReference_SameArgsTwice_ResolvesOnceAndSharesInstances() {