From f0671792fc84d21a7378e18d457ed4c35a008813 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 30 Sep 2026 17:01:39 -0400 Subject: [PATCH] fix(manifest): a document parsed on two threads at once could be rejected as unparseable PolicyLoader and ManifestLoader each held a process-wide static YamlDotNet deserializer. YamlDotNet 13.7.1 (Directory.Packages.props) is not safe to share across threads during first use: each deserializer's DefaultObjectFactory caches per-type [OnDeserializing]/[OnDeserialized] lookups in a plain Dictionary and writes it, unlocked, the first time it meets each type. Two cold first parses at the same moment corrupt it; the exception is wrapped as "Exception during deserialization", and the loader rejects a VALID policy or manifest as one that "could not be parsed". Upstream fixed it with a ConcurrentDictionary in YamlDotNet 16.1.0 (not in 16.0.0). Measured, not inferred: - CI: 4 red readiness runs in 349 with a test sweep (~1.1%), 2 of them master pushes - SelfExtendAdmissionBridgeTests on macOS (run 36740078979), ManifestContractTests on Linux (35517434376) and Windows (35487097796), and ProjectVerifierTests through ManifestLoader (34062198636). It passed on the PR run of the same tree, which is what made it look like a macOS flake. - Reproduced in the devtest container: 10 of 6000 concurrent cold parses rejected; 0 with a warm cache. The four real policy-parsing test classes reproduce the CI message word for word on the base tree. - Production: any process parsing two documents at once for the first time - concurrent self-extend cycles at a host's cold start - got a spurious fail-closed GATE ERROR. Closed, not open, but a real availability bug. Fix: both loaders build a new deserializer per parse. Caching the builder instead is NOT enough: DeserializerBuilder hands one lazily created object factory to every Build() (mutation measured red 20/20). A document is parsed once per command or self-extend cycle, so the cost is irrelevant. Two tests, because one cannot see everything: - PolicyLoaderColdStartConcurrencyTests loads fresh copies of Ashlar.Manifest and YamlDotNet into a collectible AssemblyLoadContext per trial, so every static starts cold, and releases 8 threads through one barrier. Base: red 20/20. Fix: green 150/150, and 0 of 9900 natural test executions failed. It cannot see a LAZILY assigned static (x ??= new ...): the barrier releases every thread while the field is null, so nothing collides in the test while production still shares one instance - that mutation stayed green 20/20. - YamlDeserializerNotSharedConventionTests closes that gap structurally: no static field anywhere in Ashlar.Manifest may hold a YamlDotNet type, checking element types and generic arguments recursively, with a non-vacuity floor on the types scanned and a second fact pinning the predicate against every caching shape (deserializer, builder, object factory, Lazy<>, Func<>, arrays, dictionaries) and against non-YAML types. Mutations observed red: a lazy ??= static in PolicyLoader, and an eager static reintroduced in ManifestLoader. Restored: green. Verification in the devtest container at this tree: Ashlar.Tests.Kernel 414/414 (412 + the 2 new facts); the cold-start test 2/2; the whole Ashlar.Tests.BackgroundAgents project 590/591 with its one existing skip, net8.0 and net10.0; cert-gate 1288/1288 on the fix's first revision. Not changed: YamlDotNet stays pinned at 13.7.1. A per-parse deserializer is correct at any version; moving past 16.1.0 is a separate decision. Co-Authored-By: Claude Opus 5.5 --- docs/knowledge-graph.json | 18 +- docs/knowledge-graph.md | 4 +- src/Ashlar.Manifest/ManifestLoader.cs | 16 +- src/Ashlar.Manifest/PolicyLoader.cs | 27 ++- .../PolicyLoaderColdStartConcurrencyTests.cs | 216 ++++++++++++++++++ ...amlDeserializerNotSharedConventionTests.cs | 105 +++++++++ 6 files changed, 376 insertions(+), 10 deletions(-) create mode 100644 src/Ashlar.Tests.BackgroundAgents/SelfExtend/PolicyLoaderColdStartConcurrencyTests.cs create mode 100644 src/Ashlar.Tests.Kernel/YamlDeserializerNotSharedConventionTests.cs diff --git a/docs/knowledge-graph.json b/docs/knowledge-graph.json index e4054abd5..73cf50c11 100644 --- a/docs/knowledge-graph.json +++ b/docs/knowledge-graph.json @@ -15,8 +15,8 @@ "operator_facing_config_variables": 153, "operator_vars_missing_from_configuration_reference": 18, "operator_vars_absent_from_all_documentation": 0, - "test_files_declaring_facts": 731, - "declared_facts": 4399, + "test_files_declaring_facts": 733, + "declared_facts": 4403, "workflows": 62 }, "operator_vars_missing_from_configuration_reference": [ @@ -3769,6 +3769,13 @@ "declared_facts": 8, "in_cert_gate_filter": false }, + { + "file": "src/Ashlar.Tests.BackgroundAgents/SelfExtend/PolicyLoaderColdStartConcurrencyTests.cs", + "project": "Ashlar.Tests.BackgroundAgents", + "namespace": "Ashlar.Tests.BackgroundAgents.SelfExtend", + "declared_facts": 2, + "in_cert_gate_filter": false + }, { "file": "src/Ashlar.Tests.BackgroundAgents/SelfExtend/SelfExtendAdmissionBridgeTests.cs", "project": "Ashlar.Tests.BackgroundAgents", @@ -7255,6 +7262,13 @@ "declared_facts": 3, "in_cert_gate_filter": false }, + { + "file": "src/Ashlar.Tests.Kernel/YamlDeserializerNotSharedConventionTests.cs", + "project": "Ashlar.Tests.Kernel", + "namespace": "Ashlar.Tests.Kernel", + "declared_facts": 2, + "in_cert_gate_filter": false + }, { "file": "src/Ashlar.Tests.Orchestration/Agents/AgentFactoryTests.cs", "project": "Ashlar.Tests.Orchestration", diff --git a/docs/knowledge-graph.md b/docs/knowledge-graph.md index b0daaf38b..2a0d97a68 100644 --- a/docs/knowledge-graph.md +++ b/docs/knowledge-graph.md @@ -19,8 +19,8 @@ remembered to update — staleness here is a red build. | of those, operator-facing (not test-only) | 153 | | missing from `docs/Configuration.md` | 18 | | **mentioned in no document at all** | **0** | -| test files declaring xUnit facts | 731 | -| declared facts | 4399 | +| test files declaring xUnit facts | 733 | +| declared facts | 4403 | | workflows | 62 | ## Namespaces cert-gate selects diff --git a/src/Ashlar.Manifest/ManifestLoader.cs b/src/Ashlar.Manifest/ManifestLoader.cs index 789971283..2087b9d83 100644 --- a/src/Ashlar.Manifest/ManifestLoader.cs +++ b/src/Ashlar.Manifest/ManifestLoader.cs @@ -34,12 +34,20 @@ public static class ManifestLoader "policy", ]; - private static readonly IDeserializer Deserializer = new DeserializerBuilder() + /// + /// NEW deserializers for every parse, for the reason given on + /// PolicyLoader.NewDeserializer: a YamlDotNet 13.7.1 deserializer is not safe to share + /// across threads during its first parses, and a shared one made this loader reject valid + /// manifests as "could not be parsed" (seen in ProjectVerifierTests, run 34062198636). + /// YamlDeserializerNotSharedConventionTests fails if any static field in this assembly + /// holds a YamlDotNet type again. + /// + private static IDeserializer NewDeserializer() => new DeserializerBuilder() .WithNamingConvention(CamelCaseNamingConvention.Instance) .IgnoreUnmatchedProperties() .Build(); - private static readonly IDeserializer RawDeserializer = new DeserializerBuilder() + private static IDeserializer NewRawDeserializer() => new DeserializerBuilder() .IgnoreUnmatchedProperties() .Build(); @@ -71,7 +79,7 @@ public static bool TryLoad(string? yaml, out AshlarManifest? manifest, out strin Dictionary? raw; try { - raw = RawDeserializer.Deserialize>(yaml!); + raw = NewRawDeserializer().Deserialize>(yaml!); } catch (YamlException ex) { @@ -97,7 +105,7 @@ public static bool TryLoad(string? yaml, out AshlarManifest? manifest, out strin AshlarManifest? parsed; try { - parsed = Deserializer.Deserialize(yaml!); + parsed = NewDeserializer().Deserialize(yaml!); } catch (YamlException ex) { diff --git a/src/Ashlar.Manifest/PolicyLoader.cs b/src/Ashlar.Manifest/PolicyLoader.cs index 85385cd23..5a457e265 100644 --- a/src/Ashlar.Manifest/PolicyLoader.cs +++ b/src/Ashlar.Manifest/PolicyLoader.cs @@ -41,7 +41,30 @@ public static class PolicyLoader /// public static readonly IReadOnlyList AddableKinds = ["brick"]; - private static readonly IDeserializer Deserializer = new DeserializerBuilder() + /// + /// A NEW deserializer for every parse. Never make this a shared static again. + /// + /// YamlDotNet 13.7.1 (Directory.Packages.props) is not safe to share across threads. + /// Each deserializer owns one DefaultObjectFactory, which caches every type's + /// [OnDeserializing]/[OnDeserialized] lookup in a plain Dictionary and + /// writes it, unlocked, the first time it meets that type. When this was one static, two + /// threads doing their first parse of a process at the same moment wrote it together; the + /// corrupted insert threw, YamlDotNet wrapped that as "Exception during deserialization", and + /// this loader rejected a VALID policy as one that "could not be parsed". The self-extend gate + /// then failed closed on it. That was the intermittent red of + /// SelfExtendAdmissionBridgeTests and ManifestContractTests in the readiness + /// gate, and any process parsing two policies at once for the first time could hit it. + /// + /// Caching the instead would NOT help: a builder hands + /// the same lazily created object factory to every Build(). Upstream made the cache + /// concurrent in YamlDotNet 16.1.0. A policy is parsed once per command or self-extend cycle, so a + /// fresh deserializer costs nothing that matters, and with nothing shared there is nothing to + /// race on. YamlDeserializerNotSharedConventionTests fails if any static field in this + /// assembly holds a YamlDotNet type, which covers every caching shape. The race itself is + /// reproduced by PolicyLoaderColdStartConcurrencyTests, which catches an eagerly shared + /// deserializer or builder but not a lazily assigned one. + /// + private static IDeserializer NewDeserializer() => new DeserializerBuilder() .WithNamingConvention(CamelCaseNamingConvention.Instance) .Build(); @@ -71,7 +94,7 @@ public static bool TryLoad(string? yaml, out AshlarPolicy? policy, out string re AshlarPolicy? parsed; try { - parsed = Deserializer.Deserialize(yaml!); + parsed = NewDeserializer().Deserialize(yaml!); } catch (YamlException ex) { diff --git a/src/Ashlar.Tests.BackgroundAgents/SelfExtend/PolicyLoaderColdStartConcurrencyTests.cs b/src/Ashlar.Tests.BackgroundAgents/SelfExtend/PolicyLoaderColdStartConcurrencyTests.cs new file mode 100644 index 000000000..2c25f61cc --- /dev/null +++ b/src/Ashlar.Tests.BackgroundAgents/SelfExtend/PolicyLoaderColdStartConcurrencyTests.cs @@ -0,0 +1,216 @@ +using System.Reflection; +using System.Runtime.Loader; +using Ashlar.Manifest; +using FluentAssertions; +using Xunit; + +namespace Ashlar.Tests.BackgroundAgents.SelfExtend; + +/// +/// Two threads parsing a policy for the first time in a process must both get the policy. +/// +/// Why this exists. The self-extend gate fails closed on a policy it cannot parse, +/// so a spurious parse failure is a spurious rejection. That is what reddened +/// +/// on the macOS lane of readiness run 36740078979 with "GATE ERROR: REJECTED: policy could not be +/// parsed: Exception during deserialization", and ManifestContractTests on Linux and +/// Windows before it. held ONE static YamlDotNet deserializer for the +/// whole process. In YamlDotNet 13.7.1 that deserializer's object factory caches each type's +/// [OnDeserializing]/[OnDeserialized] lookup in a plain Dictionary that it +/// writes, unlocked, the first time it meets the type. Two test classes (or two self-extend +/// cycles) doing their first parse at the same instant wrote it together; the corrupted insert +/// threw, and YamlDotNet wrapped that as "Exception during deserialization". +/// +/// Why a process start has to be rebuilt here. The cache is written only on a miss, +/// so the race exists only while it is cold: the first parses of a process. Re-running any test +/// in a process that has already parsed a policy can never see it, which is why the original +/// failure was ~0.1% per lane. Each trial below therefore loads FRESH copies of +/// Ashlar.Manifest and YamlDotNet into their own collectible +/// , where every static they own starts empty exactly as it does +/// at process start, and releases all the threads into PolicyLoader.TryLoad through one +/// barrier. With a shared deserializer restored it failed nearly every run when measured, pinned +/// to two cores included; with no shared state there is nothing to race on and it cannot fail. +/// +/// What this cannot see. Only . ManifestLoader keeps +/// two static deserializers of the same kind and is not exercised here. +/// +public sealed class PolicyLoaderColdStartConcurrencyTests +{ + /// More threads than any CI runner has cores: the barrier, not the core count, lines them up. + private const int Threads = 8; + + /// + /// Cold starts per run. Pinned to two cores, 20 trials still missed a restored shared + /// deserializer in 2 runs of 20, so a small runner gets twice that. + /// + private const int Trials = 40; + + /// Byte-for-byte what SelfExtendAdmissionBridgeTests.WritePolicy("self-extending") writes. + private const string Yaml = """ + apiVersion: ashlar/v1 + kind: Policy + sandbox: + root: . + writable: [] + selfExtend: + mode: self-extending + budget: + extensions: 3 + window: 24h + mayAdd: [brick] + gatesRequired: [sandbox] + never: + - modify_gate + - widen_sandbox + - access_signing_keys + - truncate_ledger + - grant_capability + """; + + [Fact] + public void Concurrent_first_parses_of_a_process_all_get_the_policy() + { + var failures = new List(); + var calls = 0; + + for (var trial = 0; trial < Trials; trial++) + { + var context = new ColdStartContext(); + try + { + var tryLoad = context.PolicyLoaderTryLoad(); + var results = RaceFirstParses(tryLoad); + calls += results.Length; + failures.AddRange(results.Where(r => r is not null).Select(r => $"trial {trial}: {r}")); + + // The trial proves nothing unless YamlDotNet itself was a fresh copy too, not the + // default context's warm one. + context.FreshAssemblyNames().Should().Contain( + ColdStartContext.Fresh, + "every statically held YamlDotNet cache must start cold, as at process start"); + } + finally + { + context.Unload(); + } + } + + calls.Should().Be(Threads * Trials, "every thread must have reached TryLoad and reported"); + failures.Should().BeEmpty( + "a valid policy must parse however many threads meet it first, yet {0} of {1} first parses " + + "were rejected; a shared, unsynchronized deserializer rejects it at random, and the gate " + + "then fails closed on a policy that is fine", + failures.Count, calls); + } + + [Fact] + public void The_cold_start_context_really_is_a_second_copy_of_the_loader() + { + // Non-vacuity for the fact above: if the load context quietly resolved to the default + // context's assemblies, every trial would share one warm deserializer and pass for the + // wrong reason. + var context = new ColdStartContext(); + try + { + var tryLoad = context.PolicyLoaderTryLoad(); + + var freshLoader = tryLoad.DeclaringType!.Assembly; + freshLoader.Should().NotBeSameAs(typeof(PolicyLoader).Assembly); + AssemblyLoadContext.GetLoadContext(freshLoader).Should().BeSameAs(context); + + var args = new object?[] { Yaml, null, null }; + ((bool)tryLoad.Invoke(null, args)!).Should().BeTrue((string?)args[2]); + args[1].Should().NotBeNull().And.NotBeOfType( + "the policy type must come from the fresh copy of Ashlar.Manifest"); + + context.FreshAssemblyNames().Should().BeEquivalentTo(ColdStartContext.Fresh); + } + finally + { + context.Unload(); + } + } + + /// Releases threads into TryLoad at once; null means that call got the policy. + private static string?[] RaceFirstParses(MethodInfo tryLoad) + { + var results = new string?[Threads]; + var reported = new bool[Threads]; + using var barrier = new Barrier(Threads); + var workers = new Thread[Threads]; + for (var i = 0; i < Threads; i++) + { + var slot = i; + workers[i] = new Thread(() => + { + barrier.SignalAndWait(); + try + { + var args = new object?[] { Yaml, null, null }; + var loaded = (bool)tryLoad.Invoke(null, args)!; + results[slot] = loaded && args[1] is not null + ? null + : (string?)args[2] ?? "TryLoad returned no policy and no reason"; + } + catch (Exception ex) + { + results[slot] = "TryLoad threw " + (ex is TargetInvocationException { InnerException: { } inner } ? inner : ex); + } + reported[slot] = true; + }) + { IsBackground = true }; + } + + foreach (var worker in workers) + { + worker.Start(); + } + foreach (var worker in workers) + { + worker.Join(); + } + + reported.Should().OnlyContain(r => r, "a thread that never reported proves nothing"); + return results; + } + + /// + /// A collectible load context holding its own copies of the assemblies whose statics make up + /// "the first parse of a process". Everything else resolves to the default context. + /// + private sealed class ColdStartContext : AssemblyLoadContext + { + public static readonly string[] Fresh = ["Ashlar.Manifest", "YamlDotNet"]; + + private readonly string _directory; + + public ColdStartContext() + : base("policy-cold-start", isCollectible: true) + { + var location = typeof(PolicyLoader).Assembly.Location; + location.Should().NotBeNullOrEmpty("the loader must be loadable from a file for a second copy to exist"); + _directory = Path.GetDirectoryName(location)!; + } + + public MethodInfo PolicyLoaderTryLoad() + { + var assembly = LoadFromAssemblyName(typeof(PolicyLoader).Assembly.GetName()); + var loader = assembly.GetType(typeof(PolicyLoader).FullName!, throwOnError: true)!; + return loader.GetMethod(nameof(PolicyLoader.TryLoad), BindingFlags.Public | BindingFlags.Static) + ?? throw new InvalidOperationException("PolicyLoader.TryLoad not found on the fresh copy"); + } + + public IEnumerable FreshAssemblyNames() => + Assemblies.Select(a => a.GetName().Name!).Where(n => Fresh.Contains(n, StringComparer.Ordinal)); + + protected override Assembly? Load(AssemblyName assemblyName) + { + if (assemblyName.Name is { } name && Fresh.Contains(name, StringComparer.Ordinal)) + { + return LoadFromAssemblyPath(Path.Combine(_directory, name + ".dll")); + } + + return null; + } + } +} diff --git a/src/Ashlar.Tests.Kernel/YamlDeserializerNotSharedConventionTests.cs b/src/Ashlar.Tests.Kernel/YamlDeserializerNotSharedConventionTests.cs new file mode 100644 index 000000000..015cf4071 --- /dev/null +++ b/src/Ashlar.Tests.Kernel/YamlDeserializerNotSharedConventionTests.cs @@ -0,0 +1,105 @@ +using System.Reflection; +using Ashlar.Manifest; +using FluentAssertions; +using Xunit; + +namespace Ashlar.Tests.Kernel; + +/// +/// No static field in Ashlar.Manifest may hold a YamlDotNet type. +/// +/// A YamlDotNet 13.7.1 deserializer caches per-type lookups in a plain Dictionary that it +/// writes, unlocked, on the first parse of each type. Shared across threads, two cold first parses +/// corrupt it, and the loader rejects a VALID policy or manifest as "could not be parsed". That is the +/// intermittent red of SelfExtendAdmissionBridgeTests, ManifestContractTests and +/// ProjectVerifierTests in the readiness gate, and a spurious fail-closed rejection in any +/// process that parses two documents at once for the first time. Both loaders now build a new +/// deserializer per parse. +/// +/// Why a structural rule and not only the race test. +/// PolicyLoaderColdStartConcurrencyTests reproduces the race, and catches an eagerly shared +/// deserializer or a cached builder. It does not catch a lazily assigned one +/// (x ??= new DeserializerBuilder()...Build()): its barrier releases every thread while the field +/// is still null, so each thread builds its own and nothing collides - yet in production the first caller +/// to finish publishes one instance that later cold callers share. Measured: that mutation stayed green 20 +/// of 20 runs. Every caching shape needs a field, and every such field is found here, so this rule has no +/// such gap. A per-parse deserializer costs nothing that matters: a document is parsed once per command +/// or self-extend cycle. +/// +public sealed class YamlDeserializerNotSharedConventionTests +{ + private const string YamlDotNet = "YamlDotNet"; + + private static readonly Assembly ManifestAssembly = typeof(PolicyLoader).Assembly; + + /// 41 types were declared in src/Ashlar.Manifest on 2026-09-30. The floor sits + /// below that so ordinary additions and removals never trip it; it exists to fail a scan of the + /// wrong or an empty assembly, which would report "no offenders" having looked at nothing. + private const int MinimumTypesScanned = 30; + + [Fact] + public void No_static_field_in_Ashlar_Manifest_holds_a_YamlDotNet_type() + { + ManifestAssembly.GetReferencedAssemblies().Select(a => a.Name) + .Should().Contain(YamlDotNet, + "the rule is only meaningful while Ashlar.Manifest parses YAML; if it stops, delete this test"); + + var types = ManifestAssembly.GetTypes(); + types.Length.Should().BeGreaterThanOrEqualTo(MinimumTypesScanned, + "a scan that finds no types proves nothing"); + + var offenders = types + .SelectMany(t => t.GetFields( + BindingFlags.Static | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.DeclaredOnly)) + .Where(f => MentionsYamlDotNet(f.FieldType)) + .Select(f => $"{f.DeclaringType!.FullName}.{f.Name} : {f.FieldType}") + .OrderBy(s => s, StringComparer.Ordinal) + .ToList(); + + offenders.Should().BeEmpty( + "a YamlDotNet 13.7.1 deserializer, builder or object factory held in a static field is shared by " + + "every thread, and two concurrent first parses corrupt its type cache so a VALID document is " + + "rejected. Build a new one per parse, as PolicyLoader.NewDeserializer and " + + "ManifestLoader.NewDeserializer do. Upstream made the cache concurrent in YamlDotNet 16.1.0; " + + "if the pin moves past that, revisit this rule rather than suppressing it."); + } + + /// The predicate must recognise every way a deserializer can be cached, or the rule above is + /// narrower than it reads. Each shape here was, or would be, a real field type. + [Fact] + public void The_predicate_recognises_every_caching_shape_and_nothing_else() + { + var yaml = Assembly.Load(new AssemblyName(YamlDotNet)); + var deserializer = yaml.GetType("YamlDotNet.Serialization.IDeserializer", throwOnError: true)!; + var builder = yaml.GetType("YamlDotNet.Serialization.DeserializerBuilder", throwOnError: true)!; + var factory = yaml.GetType("YamlDotNet.Serialization.IObjectFactory", throwOnError: true)!; + + MentionsYamlDotNet(deserializer).Should().BeTrue("an eager or ??=-assigned static deserializer"); + MentionsYamlDotNet(builder).Should().BeTrue("a cached builder shares its lazily built object factory"); + MentionsYamlDotNet(factory).Should().BeTrue("the object factory is the unsafe cache itself"); + MentionsYamlDotNet(typeof(Lazy<>).MakeGenericType(deserializer)).Should().BeTrue("Lazy"); + MentionsYamlDotNet(typeof(Func<>).MakeGenericType(deserializer)).Should().BeTrue("a cached factory delegate"); + MentionsYamlDotNet(deserializer.MakeArrayType()).Should().BeTrue("a pool"); + MentionsYamlDotNet(typeof(Dictionary<,>).MakeGenericType(typeof(string), deserializer)) + .Should().BeTrue("a keyed cache"); + + MentionsYamlDotNet(typeof(string)).Should().BeFalse(); + MentionsYamlDotNet(typeof(IReadOnlyList)).Should().BeFalse(); + MentionsYamlDotNet(typeof(Lazy>)).Should().BeFalse(); + } + + private static bool MentionsYamlDotNet(Type type) + { + if (type.Assembly.GetName().Name == YamlDotNet) + { + return true; + } + + if (type.HasElementType && MentionsYamlDotNet(type.GetElementType()!)) + { + return true; + } + + return type.IsGenericType && type.GetGenericArguments().Any(MentionsYamlDotNet); + } +}