Repository navigation
fix(manifest): a document parsed on two threads at once could be rejected as unparseable - #687
Merged
Merged
Conversation
…cted 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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The "macOS flake" in
SelfExtendAdmissionBridgeTestsis a real thread-safety bug, root-caused andreproduced — and it is in production code, not just tests.
Cause.
PolicyLoaderandManifestLoaderheld process-wide static YamlDotNet deserializers. YamlDotNet13.7.1 caches per-type lookups in a plain
Dictionaryit writes without a lock on the first parse of eachtype, so two cold first parses at the same moment corrupt it and a valid policy or manifest is rejected
as "could not be parsed". Upstream fixed it in YamlDotNet 16.1.0.
Blast radius, measured. 4 red readiness runs in 349 (~1.1%), 2 on master, across all three platforms and
three test classes. In production, concurrent self-extend cycles at a cold start could get a spurious
fail-closed
GATE ERROR— an availability bug, not a security hole.Fix. A new deserializer per parse in both loaders. Caching the builder is not enough — it shares one
lazily built object factory across
Build()calls (measured).Two tests, because one could not see everything:
PolicyLoaderColdStartConcurrencyTestsAssemblyLoadContexts so every static starts coldYamlDeserializerNotSharedConventionTestsAshlar.Manifestmay hold a YamlDotNet type??=static — which the race test cannot see — and an eager static each turn it redVerified in the devtest container: Kernel 414/414, the cold-start test 2/2, BackgroundAgents 590/591 (one
existing skip) on net8.0 and net10.0.
run-repo-gates.sh21/21, knowledge graph current.YamlDotNet stays at 13.7.1; a per-parse deserializer is correct at any version.
🤖 Generated with Claude Code