From 51c91dadbb062407a214f825a224d2bbcca148d4 Mon Sep 17 00:00:00 2001 From: lewing Date: Fri, 19 Jun 2026 16:15:22 -0500 Subject: [PATCH 1/4] Fix ObjectDisposedException race in ProjectItemInstance.TaskItem.EnumerateMetadata MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `EnumerateMetadata(ImmutableDictionary)` was a `yield return` iterator that parked an `ImmutableDictionary.Enumerator` struct copy across yield boundaries inside the compiler-generated state machine. That struct shares its traversal stack with every other copy through a `SecurePooledObject>` slot; when *any* copy is `Dispose()`'d the slot is returned to the pool and other live copies throw `ObjectDisposedException` on their next `MoveNext` call (see `SortedInt32KeyNode.Enumerator.ThrowIfDisposed` in System.Collections.Immutable). The crash is observable as MSB4166 "Child node X exited prematurely" with this inner exception: Microsoft.Build.Exceptions.InternalLoggerException: ---> System.ObjectDisposedException: Object name: 'System.Collections.Immutable.SortedInt32KeyNode`1+Enumerator[[...]]' at System.Collections.Immutable.SortedInt32KeyNode`1.Enumerator.MoveNext() at System.Collections.Immutable.ImmutableDictionary`2.Enumerator.MoveNext() at Microsoft.Build.Execution.ProjectItemInstance.TaskItem.EnumerateMetadata(ImmutableDictionary`2 list)+MoveNext() at Microsoft.Build.Logging.BuildEventArgsWriter.Write(ITaskItem item, Boolean writeMetadata) at Microsoft.Build.Logging.BuildEventArgsWriter.WriteTaskItemList(IEnumerable items, Boolean writeMetadata) at Microsoft.Build.Logging.BuildEventArgsWriter.Write(TargetFinishedEventArgs e) It happens when the same ItemDefinitions-derived metadata dictionary is serialized concurrently by the binary logger and another logger sink (or when the build engine releases the project state mid-iteration on the logger thread). It has been the long-standing root cause of dotnet/runtime#92290, which previously attributed the symptom to dotnet/msbuild#10342 (now fixed). Stack traces captured by the additional MSBuild dump logging added in dotnet/arcade#16715 surfaced this distinct inner exception in mid-2026. Fix: materialize the metadata snapshot eagerly into a heap-allocated array rather than yielding from a state-machine iterator. The single enumeration of `list` now happens entirely inside one stack frame on the caller's thread; the returned array carries no reference to a pooled enumerator and is safe to walk on any thread. A repo-wide audit found no other `yield return` iterator over an `Immutable(Dictionary|List|Sorted(Set|Dictionary)|HashSet)` in src/Build, src/Framework, or src/Tasks. The `ImmutableItemDictionary.GetCopyOnReadEnumerable` helper is the only adjacent pattern that's conceptually similar, but its backing `ICollection` is generally not a SecurePooledObject-backed type — left unchanged here pending a separate audit. Tests: existing BinaryLoggerTests (80/81 pass, 1 unrelated skip), TaskParameter_Tests (45/45), LoggingService_Tests (64/64) still pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/Build/Instance/ProjectItemInstance.cs | 29 ++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/src/Build/Instance/ProjectItemInstance.cs b/src/Build/Instance/ProjectItemInstance.cs index fcb166f65f4..af6d68a5cc6 100644 --- a/src/Build/Instance/ProjectItemInstance.cs +++ b/src/Build/Instance/ProjectItemInstance.cs @@ -1215,10 +1215,37 @@ private IEnumerable> EnumerateMetadataEager(Immutab private IEnumerable> EnumerateMetadata(ImmutableDictionary list) { + // Materialize the metadata snapshot eagerly into a heap-allocated array rather + // than yielding from a state-machine iterator. + // + // Why: ImmutableDictionary.Enumerator is a struct whose traversal stack is + // rented from a process-wide SecureObjectPool, and the pool uses struct-copy + // semantics — every copy of the Enumerator shares the same pooled Stack and the + // first copy to be Dispose()'d returns the slot to the pool. Any other live copy + // then throws ObjectDisposedException on its next MoveNext call (see + // SortedInt32KeyNode.Enumerator.ThrowIfDisposed in System.Collections.Immutable). + // + // The previous `foreach (...) { yield return ...; }` form parked an Enumerator + // copy inside the compiler-generated state machine across yield points. When + // the binary logger serialized the same item from a parallel logger sink (or + // when MSBuild's parallel build released the project state while the logger + // thread was still mid-iteration), the parked struct's pool slot was reclaimed + // and a subsequent MoveNext crashed in BuildEventArgsWriter.Write(ITaskItem), + // failing the whole build with MSB4166 / "Child node exited prematurely". + // See dotnet/runtime#92290 for the long-standing umbrella tracking this. + // + // Materializing here keeps the entire enumeration of `list` inside a single + // stack frame on the caller's thread; the returned array carries no reference + // to a pooled enumerator and is safe to walk on any thread. + var snapshot = new KeyValuePair[list.Count]; + int i = 0; foreach (KeyValuePair projectMetadataInstance in list) { - yield return new KeyValuePair(projectMetadataInstance.Key, EscapingUtilities.UnescapeAll(projectMetadataInstance.Value)); + snapshot[i++] = new KeyValuePair( + projectMetadataInstance.Key, + EscapingUtilities.UnescapeAll(projectMetadataInstance.Value)); } + return snapshot; } /// From 4c81edcf3f8513932fc35995416d5fac490cb109 Mon Sep 17 00:00:00 2001 From: lewing Date: Fri, 19 Jun 2026 16:29:57 -0500 Subject: [PATCH 2/4] Address Copilot review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Short-circuit the empty case in EnumerateMetadata(ImmutableDictionary) to avoid allocating a 0-length array for items with no custom metadata (common path). - Update the stale comment in the public EnumerateMetadata() to reflect the new rationale — the call is no longer about avoiding allocation, it's about not letting pooled ImmutableDictionary.Enumerator state escape the call frame. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/Build/Instance/ProjectItemInstance.cs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/Build/Instance/ProjectItemInstance.cs b/src/Build/Instance/ProjectItemInstance.cs index af6d68a5cc6..dd5a16ea069 100644 --- a/src/Build/Instance/ProjectItemInstance.cs +++ b/src/Build/Instance/ProjectItemInstance.cs @@ -1126,9 +1126,12 @@ public IEnumerable> EnumerateMetadata() return EnumerateMetadataEager(list); } #endif - // Mainline scenario, returns an iterator to avoid allocating an array - // to store the results. With the iterator, results can stream to the - // consumer (e.g. binlog writer) without allocations. + // Mainline scenario: materialize a snapshot eagerly. A streaming iterator + // would let pooled ImmutableDictionary.Enumerator state escape the call, + // and consumers (e.g. BuildEventArgsWriter from the logger thread) can + // race with the producing thread on the shared SecurePooledObject slot, + // producing ObjectDisposedException mid-MoveNext. See the comment in + // EnumerateMetadata(ImmutableDictionary) below for the full mechanism. return EnumerateMetadata(list); } else @@ -1237,7 +1240,12 @@ private IEnumerable> EnumerateMetadata(ImmutableDic // Materializing here keeps the entire enumeration of `list` inside a single // stack frame on the caller's thread; the returned array carries no reference // to a pooled enumerator and is safe to walk on any thread. - var snapshot = new KeyValuePair[list.Count]; + int count = list.Count; + if (count == 0) + { + return []; + } + var snapshot = new KeyValuePair[count]; int i = 0; foreach (KeyValuePair projectMetadataInstance in list) { From cdd666814b1aeb1a62e038139cdf0522271e6b39 Mon Sep 17 00:00:00 2001 From: lewing Date: Fri, 19 Jun 2026 17:34:10 -0500 Subject: [PATCH 3/4] Address review feedback on EnumerateMetadata fix - Use the existing EnumerateMetadataEager helper instead of introducing a new EnumerateMetadata(ImmutableDictionary) wrapper (rainersigwald). - Lift EnumerateMetadataEager out of #if FEATURE_APPDOMAIN; the eager materialization is what makes the fix correct on the mainline path too. - Tighten the body to a single pre-sized array allocation (was List+ToArray). - Apply the same fix to Microsoft.Build.Utilities.TaskItem, which had an identical yield-return-over-ImmutableDictionary pattern in EnumerateMetadataLazy (caught by the automated review). - Add structural regression tests on both TaskItem types asserting that EnumerateMetadata returns a materialized snapshot, not a compiler-generated iterator state machine. This makes a future revert of the fix observable without having to reproduce the race. - Soften the rationale comment: the exact mechanism (which copy of the pooled enumerator struct interacts with logger serialization) is not fully proven; what the fix does prove out is that eager materialization removes the entire class of issues by ending the lifetime of the pooled enumerator inside a single stack frame. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Instance/TaskItem_Tests.cs | 29 ++++++++ src/Build/Instance/ProjectItemInstance.cs | 74 ++++--------------- src/Utilities.UnitTests/TaskItem_Tests.cs | 24 ++++++ src/Utilities/TaskItem.cs | 45 +++++------ 4 files changed, 84 insertions(+), 88 deletions(-) diff --git a/src/Build.UnitTests/Instance/TaskItem_Tests.cs b/src/Build.UnitTests/Instance/TaskItem_Tests.cs index 7e6fd0f64f4..ec10d0d205b 100644 --- a/src/Build.UnitTests/Instance/TaskItem_Tests.cs +++ b/src/Build.UnitTests/Instance/TaskItem_Tests.cs @@ -373,5 +373,34 @@ public void Escaping3() logger.AssertLogContains("i1%2ai2"); } + + /// + /// Regression test for the ObjectDisposedException race rooted in returning a + /// `yield return` iterator from EnumerateMetadata over an ImmutableDictionary. + /// See dotnet/runtime#92290 and the EnumerateMetadataEager rationale comment. + /// + /// The intent of the fix is that callers always receive a materialized snapshot + /// (an array), never a compiler-generated state-machine iterator that parks a + /// copy of the ImmutableDictionary.Enumerator struct across yield boundaries. + /// Asserting on the runtime type makes a future revert of the fix observable. + /// + [Fact] + public void EnumerateMetadata_ReturnsMaterializedSnapshot_NotYieldIterator() + { + ProjectItemInstance.TaskItem item = new("foo", "test.proj"); + item.SetMetadata("a", "1"); + item.SetMetadata("b", "2"); + + IEnumerable> result = ((IMetadataContainer)item).EnumerateMetadata(); + + // The result must NOT be a compiler-generated yield-return state machine. Those box + // a copy of the source ImmutableDictionary.Enumerator into a heap field, and that + // copy's pooled traversal stack outlives the call frame in ways that have been + // observed to interact with logger serialization and produce ObjectDisposedException. + result.GetType().FullName.ShouldNotContain("> EnumerateMetadata() // If we have item definitions, call the expensive property that does the right thing. // Otherwise use _directMetadata to avoid allocations caused by DeepClone(). var list = _itemDefinitions != null ? MetadataCollection : DirectMetadata; - if (list != null) - { -#if FEATURE_APPDOMAIN - // Can't send a yield-return iterator across AppDomain boundaries - if (!AppDomain.CurrentDomain.IsDefaultAppDomain()) - { - return EnumerateMetadataEager(list); - } -#endif - // Mainline scenario: materialize a snapshot eagerly. A streaming iterator - // would let pooled ImmutableDictionary.Enumerator state escape the call, - // and consumers (e.g. BuildEventArgsWriter from the logger thread) can - // race with the producing thread on the shared SecurePooledObject slot, - // producing ObjectDisposedException mid-MoveNext. See the comment in - // EnumerateMetadata(ImmutableDictionary) below for the full mechanism. - return EnumerateMetadata(list); - } - else - { - return []; - } + return list != null ? EnumerateMetadataEager(list) : []; } /// @@ -1195,51 +1175,25 @@ private void ImportMetadata(IEnumerable> metadata, } } -#if FEATURE_APPDOMAIN /// - /// Used to return metadata from another AppDomain. Can't use yield return because the - /// generated state machine is not marked as [Serializable], so we need to allocate. + /// Materialize the metadata snapshot eagerly into a heap-allocated array. /// + /// + /// Used historically only on the AppDomain-crossing path because a yield-iterator + /// state machine is not [Serializable]. Now used unconditionally to avoid letting + /// a copy of the pooled + /// struct escape the call frame inside a compiler-generated iterator. That state + /// machine has a complex lifetime that has been observed to interact with logger + /// serialization to produce ObjectDisposedException in + /// , surfacing as + /// MSB4166 "Child node exited prematurely" — see dotnet/runtime#92290. Eager + /// materialization removes the entire class of issues by returning a regular + /// array that has no relationship to the BCL enumerator pool. + /// /// The source list to return metadata from. /// An array of string key-value pairs representing metadata. private IEnumerable> EnumerateMetadataEager(ImmutableDictionary list) { - var result = new List>(list.Count); - - foreach (KeyValuePair projectMetadataInstance in list) - { - result.Add(new KeyValuePair(projectMetadataInstance.Key, EscapingUtilities.UnescapeAll(projectMetadataInstance.Value))); - } - - // Probably better to send the raw array across the wire even if it's another allocation. - return result.ToArray(); - } -#endif - - private IEnumerable> EnumerateMetadata(ImmutableDictionary list) - { - // Materialize the metadata snapshot eagerly into a heap-allocated array rather - // than yielding from a state-machine iterator. - // - // Why: ImmutableDictionary.Enumerator is a struct whose traversal stack is - // rented from a process-wide SecureObjectPool, and the pool uses struct-copy - // semantics — every copy of the Enumerator shares the same pooled Stack and the - // first copy to be Dispose()'d returns the slot to the pool. Any other live copy - // then throws ObjectDisposedException on its next MoveNext call (see - // SortedInt32KeyNode.Enumerator.ThrowIfDisposed in System.Collections.Immutable). - // - // The previous `foreach (...) { yield return ...; }` form parked an Enumerator - // copy inside the compiler-generated state machine across yield points. When - // the binary logger serialized the same item from a parallel logger sink (or - // when MSBuild's parallel build released the project state while the logger - // thread was still mid-iteration), the parked struct's pool slot was reclaimed - // and a subsequent MoveNext crashed in BuildEventArgsWriter.Write(ITaskItem), - // failing the whole build with MSB4166 / "Child node exited prematurely". - // See dotnet/runtime#92290 for the long-standing umbrella tracking this. - // - // Materializing here keeps the entire enumeration of `list` inside a single - // stack frame on the caller's thread; the returned array carries no reference - // to a pooled enumerator and is safe to walk on any thread. int count = list.Count; if (count == 0) { diff --git a/src/Utilities.UnitTests/TaskItem_Tests.cs b/src/Utilities.UnitTests/TaskItem_Tests.cs index 6e0cc3a9afa..8b4498c3ce3 100644 --- a/src/Utilities.UnitTests/TaskItem_Tests.cs +++ b/src/Utilities.UnitTests/TaskItem_Tests.cs @@ -342,6 +342,30 @@ public void ImplementsIMetadataContainer() Assert.True(actualMetadata.SequenceEqual(expectedMetadata)); } + /// + /// Regression test for the ObjectDisposedException race rooted in returning a + /// `yield return` iterator from EnumerateMetadata over an ImmutableDictionary. + /// See dotnet/runtime#92290. + /// + /// EnumerateMetadata must return a materialized snapshot (an array), not a + /// compiler-generated state-machine iterator that parks a copy of the + /// ImmutableDictionary.Enumerator struct (and its pooled traversal stack) + /// across yield boundaries. + /// + [Fact] + public void EnumerateMetadata_ReturnsMaterializedSnapshot_NotYieldIterator() + { + TaskItem item = new TaskItem("foo"); + ((IMetadataContainer)item).ImportMetadata(new Dictionary { { "a", "1" }, { "b", "2" } }); + + IEnumerable> result = ((IMetadataContainer)item).EnumerateMetadata(); + + result.GetType().FullName.ShouldNotContain(" /// Test that task items can be successfully constructed based on a task item from another appdomain. diff --git a/src/Utilities/TaskItem.cs b/src/Utilities/TaskItem.cs index c25f4cf6148..0f253c370cc 100644 --- a/src/Utilities/TaskItem.cs +++ b/src/Utilities/TaskItem.cs @@ -539,18 +539,15 @@ IDictionary ITaskItem2.CloneCustomMetadataEscaped() => _metadata == null IEnumerable> IMetadataContainer.EnumerateMetadata() { -#if FEATURE_APPDOMAIN - // Can't send a yield-return iterator across AppDomain boundaries - // so have to allocate - if (!AppDomain.CurrentDomain.IsDefaultAppDomain()) - { - return EnumerateMetadataEager(); - } -#endif - - // In general case we want to return an iterator without allocating a collection - // to hold the result, so we can stream the items directly to the consumer. - return EnumerateMetadataLazy(); + // Always materialize eagerly. A yield-return iterator over an ImmutableDictionary + // parks a copy of the dictionary's pooled enumerator struct inside a heap-allocated + // state machine whose lifetime is harder to reason about than a one-shot snapshot; + // it has been observed to interact with logger serialization to produce + // ObjectDisposedException, surfacing as MSB4166 "Child node exited prematurely" + // (dotnet/runtime#92290). Eager materialization removes the entire class of issues + // by returning a regular array. Same reasoning as the + // Microsoft.Build.Execution.ProjectItemInstance.TaskItem.EnumerateMetadata twin. + return EnumerateMetadataEager(); } void IMetadataContainer.ImportMetadata(IEnumerable> metadata) @@ -565,7 +562,14 @@ void IMetadataContainer.ImportMetadata(IEnumerable> _metadata = _metadata.SetItems(metadata.Select(kvp => new KeyValuePair(kvp.Key, kvp.Value ?? string.Empty))); } -#if FEATURE_APPDOMAIN + /// + /// Materialize the metadata snapshot eagerly into a heap-allocated array. + /// + /// + /// Used historically only on the AppDomain-crossing path because a yield-iterator state + /// machine is not [Serializable]. Now used unconditionally — see the + /// IMetadataContainer.EnumerateMetadata implementation above for the rationale. + /// private IEnumerable> EnumerateMetadataEager() { if (_metadata == null) @@ -584,20 +588,5 @@ private IEnumerable> EnumerateMetadataEager() return result; } -#endif - - private IEnumerable> EnumerateMetadataLazy() - { - if (_metadata == null) - { - yield break; - } - - foreach (var kvp in _metadata) - { - var unescaped = new KeyValuePair(kvp.Key, EscapingUtilities.UnescapeAll(kvp.Value)); - yield return unescaped; - } - } } } From 8fbc83ee011787b73b798c311e3ef2ff60f21fa7 Mon Sep 17 00:00:00 2001 From: lewing Date: Fri, 19 Jun 2026 18:14:52 -0500 Subject: [PATCH 4/4] Tighten EnumerateMetadata regression tests Per rubber-duck review of the previous commit: - Replace name-based runtime-type substring check with the stronger ShouldBeOfType[]>(): asserts the invariant the fix is actually about (caller receives a materialized array), not the absence of a specific generated-iterator name. - Add a snapshot-after-mutation behavioural assertion: mutate the item after EnumerateMetadata returned and verify the returned sequence still reflects the pre-mutation state. A streaming iterator over _directMetadata would observe the new value. - Keep the empty-case short-circuit assertion. Verified the strengthened tests still fail against the pre-fix source on both TaskItem types. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Instance/TaskItem_Tests.cs | 23 ++++++++++++++----- src/Utilities.UnitTests/TaskItem_Tests.cs | 14 ++++++++++- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/src/Build.UnitTests/Instance/TaskItem_Tests.cs b/src/Build.UnitTests/Instance/TaskItem_Tests.cs index ec10d0d205b..a593b67d63c 100644 --- a/src/Build.UnitTests/Instance/TaskItem_Tests.cs +++ b/src/Build.UnitTests/Instance/TaskItem_Tests.cs @@ -393,12 +393,23 @@ public void EnumerateMetadata_ReturnsMaterializedSnapshot_NotYieldIterator() IEnumerable> result = ((IMetadataContainer)item).EnumerateMetadata(); - // The result must NOT be a compiler-generated yield-return state machine. Those box - // a copy of the source ImmutableDictionary.Enumerator into a heap field, and that - // copy's pooled traversal stack outlives the call frame in ways that have been - // observed to interact with logger serialization and produce ObjectDisposedException. - result.GetType().FullName.ShouldNotContain("[]>(); + + // Behavioural invariant: the snapshot must not observe later mutations of the item. + // A streaming iterator would re-enumerate _directMetadata on iteration, exposing + // mutations made after EnumerateMetadata returned. + item.SetMetadata("a", "changed-after-snapshot"); + item.SetMetadata("c", "added-after-snapshot"); + result.ShouldBe(new[] + { + new KeyValuePair("a", "1"), + new KeyValuePair("b", "2"), + }, ignoreOrder: true); + + // No-custom-metadata case short-circuits to Array.Empty<>. ProjectItemInstance.TaskItem empty = new("bar", "test.proj"); ((IMetadataContainer)empty).EnumerateMetadata().ShouldBeEmpty(); } diff --git a/src/Utilities.UnitTests/TaskItem_Tests.cs b/src/Utilities.UnitTests/TaskItem_Tests.cs index 8b4498c3ce3..e8bb9c703f1 100644 --- a/src/Utilities.UnitTests/TaskItem_Tests.cs +++ b/src/Utilities.UnitTests/TaskItem_Tests.cs @@ -360,7 +360,19 @@ public void EnumerateMetadata_ReturnsMaterializedSnapshot_NotYieldIterator() IEnumerable> result = ((IMetadataContainer)item).EnumerateMetadata(); - result.GetType().FullName.ShouldNotContain("[]>(); + + // Behavioural invariant: the snapshot must not observe later mutations of the item. + item.SetMetadata("a", "changed-after-snapshot"); + item.SetMetadata("c", "added-after-snapshot"); + result.ShouldBe(new[] + { + new KeyValuePair("a", "1"), + new KeyValuePair("b", "2"), + }, ignoreOrder: true); TaskItem empty = new TaskItem("bar"); ((IMetadataContainer)empty).EnumerateMetadata().ShouldBeEmpty();