Preserved abandoned work from closed #2442: serialize the type-mapping specs (do not merge) - #2578
Preserved abandoned work from closed #2442: serialize the type-mapping specs (do not merge)#2578woksin wants to merge 1 commit into
Conversation
CI caught these racing where local runs did not. The mapping table is a static that SetTypeMappings replaces wholesale, so two specs configuring it at once each resolve against the other's table - and distinct subject types cannot avoid it, because the replacement takes the whole table with it. Put them in a collection with parallelization disabled. Disabled for the collection rather than only within it, since a mapping installed here is visible to any spec in the assembly that resolves a target type. Reproduced and proved: without the collection the filtered run CI uses fails 9 times in 10, with it 0 in 10. Local full-assembly runs passed throughout, which is why this reached CI - spreading the four classes across 1055 specs hid what the filtered subset makes near-certain.
Assessment — is this worth reviving?Short answer: yes, probably — this is the one of the five that is cheap, still correct, and fixes a real flakiness source. It is also the one that was most at risk of being lost silently. What it appears to doOne commit, "Serialize the specs that configure type mappings". The That is the standard, correct remedy for global-state specs under a parallel runner. It is five files, one of them new, and it touches no production code. Why this one nearly disappearedThis is the important part. #2442 recorded its head as So the closed PR is not a reliable record of what the branch contains. Worth remembering as a general hazard: a PR's head is not necessarily its branch's tip. What still applies todayAll of it, as far as I can tell. The four specs it modifies all still exist on
And The one thing to verify before revivingI have not run the specs, and I cannot tell from the tree alone whether the underlying race is still reachable — if the global registry has since been made per-instance or reset between specs, this fix would be redundant rather than wrong. Check that first. If the registry is still process-global, this should just go in. RecommendationThe most revivable of the five. Small, self-contained, no API surface, no release-note implications. The realistic path is not to merge this branch — it is 166 commits behind and the commit is trivial — but to re-apply the same change on a fresh branch off current Original review #2442. |
|
NuGet packages for this PR, e.g. Cratis.Arc: |
1 participant
Preservation only — do not merge
This is recovered abandoned work, opened as a draft purely so it is discoverable. It is not a live proposal and it is not ready to merge.
The branch went through review as #2442 ("Let a consumer declare how a type crosses the wire"), closed unmerged on 2026-08-04.
The branch carries a commit the closed PR never showed
This is the reason it is worth preserving deliberately rather than assuming #2442 already captured it.
78b37ce8("Depend on@cratis/fundamentalsas a peer") — and that commit is an ancestor ofmain, so as far as the closed PR is concerned there is nothing outstanding.originis atb514f329("Serialize the specs that configure type mappings"), one commit further on, and that commit is not inmain.In other words the substantive commit sits on the branch but was never visible in the pull request that was closed. Anyone auditing #2442 alone would wrongly conclude nothing was lost.
Why the repository PR template is not used
The template in
.github/pull_request_template.mdstates that "Release notes are generated from this description". This PR must never generate a release note — it ships nothing. The template's## Added/## Changed/## Fixedsections are therefore deliberately omitted rather than left empty.Why there is no semver label
No
major/minor/patchlabel is applied, and none should be added. A missing label is the correct and intended state: it means no release is cut, which is exactly right for a branch that exists only to preserve history. Theverifycheck will fail because the label is missing. That failure is expected and must not be "fixed".How far behind main this is
166 commits behind
main, merge base78b37ce8(2026-08-04). The rendered diff is moderate rather than enormous, but the branch is still a single commit of real content.What the work is
One commit that stops the
TypeExtensionsproxy-generator specs racing each other. Those specs mutate the process-global type-mapping registry, so running them in parallel makes them flaky. The fix introduces an xUnit collection definition and puts the four offending specs into it, forcing them to serialize.Files genuinely unique to this branch
Verified as absent from
origin/mainatd9918345:Source/DotNET/Tools/ProxyGenerator.Specs/for_TypeExtensions/TypeMappingCollectionDefinition.csPlus four spec files that exist on
mainbut are modified here to join the collection:Source/DotNET/Tools/ProxyGenerator.Specs/for_TypeExtensions/when_a_type_mapping_is_configured.csSource/DotNET/Tools/ProxyGenerator.Specs/for_TypeExtensions/when_a_type_mapping_is_configured_without_a_package.csSource/DotNET/Tools/ProxyGenerator.Specs/for_TypeExtensions/when_a_type_mapping_overrides_a_built_in_one.csSource/DotNET/Tools/ProxyGenerator.Specs/for_TypeExtensions/when_no_type_mappings_are_configured.csOriginal review: #2442