Fix ObjectDisposedException race in ProjectItemInstance.TaskItem.EnumerateMetadata

src/Build.UnitTests/Instance/TaskItem_Tests.cs

@@ -373,5 +373,45 @@ public void Escaping3()
 
             logger.AssertLogContains("i1%2ai2");
         }
+
+        /// <summary>
+        /// 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.
+        /// </summary>
+        [Fact]
+        public void EnumerateMetadata_ReturnsMaterializedSnapshot_NotYieldIterator()
+        {
+            ProjectItemInstance.TaskItem item = new("foo", "test.proj");
+            item.SetMetadata("a", "1");
+            item.SetMetadata("b", "2");
+
+            IEnumerable<KeyValuePair<string, string>> result = ((IMetadataContainer)item).EnumerateMetadata();
+
+            // Type-level invariant: result must be a materialized array, not a compiler-generated
+            // yield-return state machine that parks a copy of the ImmutableDictionary.Enumerator
+            // struct across yield boundaries.
+            result.ShouldBeOfType<KeyValuePair<string, string>[]>();
+
+            // 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<string, string>("a", "1"),
+                new KeyValuePair<string, string>("b", "2"),
+            }, ignoreOrder: true);
+
+            // No-custom-metadata case short-circuits to Array.Empty<>.
+            ProjectItemInstance.TaskItem empty = new("bar", "test.proj");
+            ((IMetadataContainer)empty).EnumerateMetadata().ShouldBeEmpty();
+        }
     }
 }

src/Build/Instance/ProjectItemInstance.cs

@@ -1117,24 +1117,7 @@ public IEnumerable<KeyValuePair<string, string>> 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, 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.
-                    return EnumerateMetadata(list);
-                }
-                else
-                {
-                    return [];
-                }
+                return list != null ? EnumerateMetadataEager(list) : [];
             }
 
             /// <summary>
@@ -1192,33 +1175,39 @@ private void ImportMetadata(IEnumerable<KeyValuePair<string, string>> metadata,
                 }
             }
 
-#if FEATURE_APPDOMAIN
             /// <summary>
-            /// 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.
             /// </summary>
+            /// <remarks>
+            /// 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 <see cref="ImmutableDictionary{TKey,TValue}.Enumerator"/>
+            /// 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
+            /// <see cref="Microsoft.Build.Logging.BuildEventArgsWriter"/>, 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.
+            /// </remarks>
             /// <param name="list">The source list to return metadata from.</param>
             /// <returns>An array of string key-value pairs representing metadata.</returns>
             private IEnumerable<KeyValuePair<string, string>> EnumerateMetadataEager(ImmutableDictionary<string, string> list)
             {
-                var result = new List<KeyValuePair<string, string>>(list.Count);
-
-                foreach (KeyValuePair<string, string> projectMetadataInstance in list)
+                int count = list.Count;
+                if (count == 0)
                 {
-                    result.Add(new KeyValuePair<string, string>(projectMetadataInstance.Key, EscapingUtilities.UnescapeAll(projectMetadataInstance.Value)));
+                    return [];
                 }
-
-                // Probably better to send the raw array across the wire even if it's another allocation.
-                return result.ToArray();
-            }
-#endif
-
-            private IEnumerable<KeyValuePair<string, string>> EnumerateMetadata(ImmutableDictionary<string, string> list)
-            {
+                var snapshot = new KeyValuePair<string, string>[count];
+                int i = 0;
                 foreach (KeyValuePair<string, string> projectMetadataInstance in list)
                 {
-                    yield return new KeyValuePair<string, string>(projectMetadataInstance.Key, EscapingUtilities.UnescapeAll(projectMetadataInstance.Value));
+                    snapshot[i++] = new KeyValuePair<string, string>(
+                        projectMetadataInstance.Key,
+                        EscapingUtilities.UnescapeAll(projectMetadataInstance.Value));
                 }
+                return snapshot;
             }
 
             /// <summary>

src/Utilities.UnitTests/TaskItem_Tests.cs

@@ -342,6 +342,42 @@ public void ImplementsIMetadataContainer()
             Assert.True(actualMetadata.SequenceEqual(expectedMetadata));
         }
 
+        /// <summary>
+        /// 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.
+        /// </summary>
+        [Fact]
+        public void EnumerateMetadata_ReturnsMaterializedSnapshot_NotYieldIterator()
+        {
+            TaskItem item = new TaskItem("foo");
+            ((IMetadataContainer)item).ImportMetadata(new Dictionary<string, string> { { "a", "1" }, { "b", "2" } });
+
+            IEnumerable<KeyValuePair<string, string>> result = ((IMetadataContainer)item).EnumerateMetadata();
+
+            // Type-level invariant: result must be a materialized array, not a compiler-generated
+            // yield-return state machine that parks a copy of the ImmutableDictionary.Enumerator
+            // struct across yield boundaries.
+            result.ShouldBeOfType<KeyValuePair<string, string>[]>();
+
+            // 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<string, string>("a", "1"),
+                new KeyValuePair<string, string>("b", "2"),
+            }, ignoreOrder: true);
+
+            TaskItem empty = new TaskItem("bar");
+            ((IMetadataContainer)empty).EnumerateMetadata().ShouldBeEmpty();
+        }
+
 #if FEATURE_APPDOMAIN
         /// <summary>
         /// Test that task items can be successfully constructed based on a task item from another appdomain.

src/Utilities/TaskItem.cs

@@ -539,18 +539,15 @@ IDictionary ITaskItem2.CloneCustomMetadataEscaped() => _metadata == null
 
         IEnumerable<KeyValuePair<string, string>> 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<KeyValuePair<string, string>> metadata)
@@ -565,7 +562,14 @@ void IMetadataContainer.ImportMetadata(IEnumerable<KeyValuePair<string, string>>
             _metadata = _metadata.SetItems(metadata.Select(kvp => new KeyValuePair<string, string>(kvp.Key, kvp.Value ?? string.Empty)));
         }
 
-#if FEATURE_APPDOMAIN
+        /// <summary>
+        /// Materialize the metadata snapshot eagerly into a heap-allocated array.
+        /// </summary>
+        /// <remarks>
+        /// 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.
+        /// </remarks>
         private IEnumerable<KeyValuePair<string, string>> EnumerateMetadataEager()
         {
             if (_metadata == null)
@@ -584,20 +588,5 @@ private IEnumerable<KeyValuePair<string, string>> EnumerateMetadataEager()
 
             return result;
         }
-#endif
-
-        private IEnumerable<KeyValuePair<string, string>> EnumerateMetadataLazy()
-        {
-            if (_metadata == null)
-            {
-                yield break;
-            }
-
-            foreach (var kvp in _metadata)
-            {
-                var unescaped = new KeyValuePair<string, string>(kvp.Key, EscapingUtilities.UnescapeAll(kvp.Value));
-                yield return unescaped;
-            }
-        }
     }
 }