From b963c24ef3657479f662347a4b1dbf8185042966 Mon Sep 17 00:00:00 2001
From: Ladi Prosek <laprosek@microsoft.com>
Date: Wed, 29 May 2024 14:47:10 +0200
Subject: [PATCH] Fix NRE in target batching (#10185)

Fixes #10180

### Context

#10102 made certain batching scenarios fail with a null-ref exception.

### Changes Made

Moved the call to `LogTargetBatchFinished` to make sure that the loop doesn't exit with null `targetLoggingContext`.

### Testing

New unit test with a repro project.
---
 .../BackEnd/BatchingEngine_Tests.cs           | 35 +++++++++++++++++++
 .../Components/RequestBuilder/TargetEntry.cs  | 17 ++++-----
 2 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/src/Build.UnitTests/BackEnd/BatchingEngine_Tests.cs b/src/Build.UnitTests/BackEnd/BatchingEngine_Tests.cs
index a0d79660060..a2a74564bb9 100644
--- a/src/Build.UnitTests/BackEnd/BatchingEngine_Tests.cs
+++ b/src/Build.UnitTests/BackEnd/BatchingEngine_Tests.cs
@@ -491,6 +491,41 @@ public void UndefinedAndEmptyMetadataValues()
             logger.AssertLogContains("[i1;i2 ]", "[i3 m1]");
         }
 
+        /// <summary>
+        /// This is a regression test for https://github.com/dotnet/msbuild/issues/10180.
+        /// </summary>
+        [Fact]
+        public void HandlesEarlyExitFromTargetBatching()
+        {
+            string content = @"
+                <Project>
+                    <ItemGroup>
+                        <Example Include='Item1'>
+                            <Color>Blue</Color>
+                        </Example>
+                        <Example Include='Item2'>
+                            <Color>Red</Color>
+                        </Example>
+                    </ItemGroup>
+
+                    <Target Name='Build'
+                        Inputs='@(Example)'
+                        Outputs='%(Color)\MyFile.txt'>
+                        <NonExistentTask
+                            Text = '@(Example)'
+                            Output = '%(Color)\MyFile.txt'/>
+                    </Target>
+                </Project>
+                ";
+
+            Project project = new Project(XmlReader.Create(new StringReader(ObjectModelHelpers.CleanupFileContents(content))));
+            MockLogger logger = new MockLogger();
+            project.Build(logger);
+
+            // Build should fail with error MSB4036: The "NonExistentTask" task was not found.
+            logger.AssertLogContains("MSB4036");
+        }
+
         private static Lookup CreateLookup(ItemDictionary<ProjectItemInstance> itemsByType, PropertyDictionary<ProjectPropertyInstance> properties)
         {
             return new Lookup(itemsByType, properties);
diff --git a/src/Build/BackEnd/Components/RequestBuilder/TargetEntry.cs b/src/Build/BackEnd/Components/RequestBuilder/TargetEntry.cs
index cc277b9d048..bd58c8a4196 100644
--- a/src/Build/BackEnd/Components/RequestBuilder/TargetEntry.cs
+++ b/src/Build/BackEnd/Components/RequestBuilder/TargetEntry.cs
@@ -452,6 +452,13 @@ internal async Task ExecuteTarget(ITaskBuilder taskBuilder, BuildRequestEntry re
                         break;
                     }
 
+                    if (i > 0)
+                    {
+                        // Don't log the last target finished event until we can process the target outputs as we want to attach them to the
+                        // last target batch. The following statement logs the event for the bucket processed in the previous iteration.
+                        targetLoggingContext.LogTargetBatchFinished(projectFullPath, targetSuccess, null);
+                    }
+
                     targetLoggingContext = projectLoggingContext.LogTargetBatchStarted(projectFullPath, _target, parentTargetName, _buildReason);
                     bucket.Initialize(targetLoggingContext);
                     WorkUnitResult bucketResult = null;
@@ -565,16 +572,6 @@ internal async Task ExecuteTarget(ITaskBuilder taskBuilder, BuildRequestEntry re
                         entryForExecution?.LeaveScope();
                         aggregateResult = aggregateResult.AggregateResult(new WorkUnitResult(WorkUnitResultCode.Failed, WorkUnitActionCode.Stop, null));
                     }
-                    finally
-                    {
-                        // Don't log the last target finished event until we can process the target outputs as we want to attach them to the
-                        // last target batch.
-                        if (targetLoggingContext != null && i < numberOfBuckets - 1)
-                        {
-                            targetLoggingContext.LogTargetBatchFinished(projectFullPath, targetSuccess, null);
-                            targetLoggingContext = null;
-                        }
-                    }
                 }
 
                 // Produce the final results.