fix(runtime): parallel-safe decompression limit; tighten ParsedDeploymentArtifact surface

2026-08-15 17:16:09 -04:00
parent 5f2e60278c
commit 14c31941c7
3 changed files with 62 additions and 35 deletions
@@ -114,7 +114,8 @@ public static class DeploymentArtifact
    /// <exception cref="JsonException">The blob is not valid JSON, is not decompressible, or inflates past
    /// <see cref="MaxDecompressedBytes"/>. Unlike the static wrappers — which own the lenient "malformed ⇒
    /// empty result" contract — opening is explicit about unusable bytes.</exception>
-    public static ParsedDeploymentArtifact Open(byte[] blob) => new(JsonDocument.Parse(Inflate(blob)));
+    internal static ParsedDeploymentArtifact Open(byte[] blob) =>
+        new(JsonDocument.Parse(Inflate(blob, MaxDecompressedBytes)));

    /// <summary>
    ///     Hard ceiling on the decompressed size of an artifact blob (512 MiB). A corrupt or hostile blob
@@ -123,10 +124,13 @@ public static class DeploymentArtifact
    ///     ones are single-digit MB before compression.
    /// </summary>
    /// <remarks>
-    ///     Mutable and internal purely so tests can lower it: proving the guard fires with the production
-    ///     value would mean crafting and inflating half a gigabyte per test run. Production never writes it.
+    ///     A <see langword="const"/> deliberately: it used to be a mutable static that the size-guard test
+    ///     lowered and restored. xUnit runs test CLASSES in parallel, so that window was visible to every
+    ///     sibling class parsing a &gt;4 KB artifact concurrently — a spurious "refusing to inflate" failure
+    ///     with no relation to the test that caused it. The limit is a <see cref="Inflate"/> parameter
+    ///     instead, so the guard is provable at any ceiling without any process-wide mutation.
    /// </remarks>
-    internal static int MaxDecompressedBytes = 512 * 1024 * 1024;
+    internal const int MaxDecompressedBytes = 512 * 1024 * 1024;

    /// <summary>
    ///     Return the artifact's JSON bytes, decompressing when the blob carries the gzip magic
@@ -150,13 +154,16 @@ public static class DeploymentArtifact
    ///     </para>
    /// </remarks>
    /// <param name="blob">The stored/wire artifact bytes.</param>
+    /// <param name="maxBytes">Ceiling on the inflated size. Production always passes
+    /// <see cref="MaxDecompressedBytes"/> via <see cref="Open"/>; the parameter exists so the guard can be
+    /// proved at a small ceiling without mutating process-wide state that parallel test classes share.</param>
    /// <returns>The JSON bytes to parse.</returns>
-    /// <exception cref="JsonException">The gzip stream is corrupt or inflates past <see cref="MaxDecompressedBytes"/>.</exception>
-    internal static byte[] Inflate(byte[] blob)
+    /// <exception cref="JsonException">The gzip stream is corrupt or inflates past <paramref name="maxBytes"/>.</exception>
+    internal static byte[] Inflate(byte[] blob, int maxBytes)
    {
        if (blob.Length < 2 || blob[0] != 0x1F || blob[1] != 0x8B) return blob;

-        var limit = MaxDecompressedBytes;
+        var limit = maxBytes;
        try
        {
            using var input = new MemoryStream(blob, writable: false);
@@ -340,8 +347,15 @@ public static class DeploymentArtifact
 ///         Not thread-safe: the per-node <see cref="ResolveClusterScope(string)"/> memo is an unsynchronised
 ///         dictionary. An apply resolves its consumers sequentially, which is the only intended use.
 ///     </para>
+///     <para>
+///         <b>Internal on purpose.</b> Every consumer is inside this assembly (the apply paths in
+///         <c>DriverHostActor</c> and <c>OpcUaPublishActor</c>, plus the tests via <c>InternalsVisibleTo</c>),
+///         and the lenient static wrappers on <see cref="DeploymentArtifact"/> are the cross-assembly door.
+///         Keeping the disposable, non-thread-safe, document-owning type unexported is what makes the
+///         "results survive disposal" contract enforceable rather than merely documented.
+///     </para>
 /// </remarks>
-public sealed class ParsedDeploymentArtifact : IDisposable
+internal sealed class ParsedDeploymentArtifact : IDisposable
 {
    private readonly JsonDocument _document;

@@ -357,8 +371,15 @@ public sealed class ParsedDeploymentArtifact : IDisposable
    /// <param name="document">The parsed artifact document, disposed with this instance.</param>
    internal ParsedDeploymentArtifact(JsonDocument document) => _document = document;

-    /// <summary>The artifact's root JSON object.</summary>
-    public JsonElement Root => _document.RootElement;
+    /// <summary>
+    ///     The artifact's root JSON object. <b>Valid only until <see cref="Dispose"/> — do not retain it,
+    ///     and do not hand it outside this type.</b> Unlike everything the parse methods return (fully
+    ///     materialised records, lists and strings that hold no reference back into the document), a
+    ///     <see cref="JsonElement"/> is a live view over the disposed-with-us <see cref="JsonDocument"/>:
+    ///     reading one after disposal throws, or worse, reads a recycled pooled buffer. Internal so that
+    ///     escape hatch cannot be taken from outside the assembly.
+    /// </summary>
+    internal JsonElement Root => _document.RootElement;

    /// <summary>Releases the underlying <see cref="JsonDocument"/>. Idempotent.</summary>
    public void Dispose()
@@ -6,8 +6,11 @@
    </PropertyGroup>

    <ItemGroup>
-        <!-- DeploymentArtifact.MaxDecompressedBytes is internal + mutable so the artifact-decompression
-             guard can be exercised without inflating half a gigabyte per test run. -->
+        <!-- ParsedDeploymentArtifact (and DeploymentArtifact.Open / .Inflate) are internal: every consumer
+             is in this assembly, and the type is a disposable, non-thread-safe wrapper over a live
+             JsonDocument that must not escape it. The tests drive that seam directly — including
+             Inflate(blob, maxBytes), which takes the decompression ceiling as a parameter so the
+             artifact-bomb guard is provable without inflating half a gigabyte per test run. -->
        <InternalsVisibleTo Include="ZB.MOM.WW.OtOpcUa.Runtime.Tests"/>
    </ItemGroup>