test(scripts): pin null-result and definition-agnostic caching; review polish

2026-08-12 16:42:27 -04:00
parent b1c3783578
commit 702de910ad
2 changed files with 82 additions and 3 deletions
@@ -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<string, PortableExecutableReference?> _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<PortableExecutableReference>> _referencesByPath = new();
+        ImmutableArray<PortableExecutableReference>> _referencesByArgs = new();

-    /// <summary>Creates a decorator over the given inner resolver. Exposed for tests; production uses <see cref="Instance"/>.</summary>
+    /// <summary>
+    /// Creates a decorator over the given inner resolver. Exposed for tests;
+    /// production code must NEVER call this constructor directly — always attach
+    /// <see cref="Instance"/>. 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.
+    /// </summary>
    /// <param name="inner">The resolver whose results are memoized.</param>
    public CachingScriptMetadataResolver(MetadataReferenceResolver inner) => _inner = inner;

@@ -79,7 +91,7 @@ public sealed class CachingScriptMetadataResolver : MetadataReferenceResolver
    /// <inheritdoc />
    public override ImmutableArray<PortableExecutableReference> ResolveReference(
        string reference, string? baseFilePath, MetadataReferenceProperties properties)
-        => _referencesByPath.GetOrAdd(
+        => _referencesByArgs.GetOrAdd(
            (reference, baseFilePath, properties),
            _ => _inner.ResolveReference(reference, baseFilePath, properties));

@@ -37,6 +37,27 @@ public class CachingScriptMetadataResolverTests
        public override int GetHashCode() => 0;
    }

+    /// <summary>Inner resolver that finds nothing — models an assembly absent from every search path.</summary>
+    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<PortableExecutableReference> 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);
    }

+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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);
+    }
+
+    /// <summary>
+    /// Locks in the documented cache-key contract: the key is the assembly
+    /// identity ALONE, and the requesting <c>definition</c> (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.
+    /// </summary>
+    [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()
    {