Canceled background steps should not impact job result (#4482)

src/Runner.Worker/BackgroundStepCoordinator.cs

@@ -32,6 +32,11 @@ namespace GitHub.Runner.Worker
         // IDs of background steps that have already been completed (waited on or canceled).
         // Used to avoid waiting on or flushing the same step more than once.
         private readonly HashSet<string> _completedStepIds = new();
+
+        // IDs of background steps that were explicitly canceled via a `cancel` control step.
+        // These steps are expected to be canceled, so their (Canceled) result must not be
+        // merged into the overall job result.
+        private readonly HashSet<string> _explicitlyCanceledStepIds = new();
         private SemaphoreSlim _backgroundSlotSemaphore = new SemaphoreSlim(DefaultMaxBackgroundSteps);
 
         /// <summary>
@@ -41,6 +46,7 @@ namespace GitHub.Runner.Worker
         {
             _backgroundSteps.Clear();
             _completedStepIds.Clear();
+            _explicitlyCanceledStepIds.Clear();
             var max = maxConcurrent > 0 ? maxConcurrent : DefaultMaxBackgroundSteps;
             _backgroundSlotSemaphore = new SemaphoreSlim(max);
         }
@@ -85,6 +91,9 @@ namespace GitHub.Runner.Worker
         // Safety net
         // -----------------------------------------------------------------
 
+        // Drain any background steps that weren't already waited on by an explicit wait/cancel
+        // control step, then merge the final results of all background steps into a single result
+        // for the caller to fold into the job result.
         public async Task<TaskResult> WaitForUnwaitedStepsAsync(CancellationToken cancellationToken)
         {
             var unwaitedIds = _backgroundSteps.Keys.Where(id => !_completedStepIds.Contains(id)).ToList();
@@ -95,14 +104,26 @@ namespace GitHub.Runner.Worker
                 CompleteWaitedSteps(unwaitedIds);
             }
 
-            // Report the merged result of all background steps; the caller merges this into the job result.
             var result = TaskResult.Succeeded;
-            foreach (var (_, (step, _, _)) in _backgroundSteps)
+            foreach (var (stepId, (step, _, _)) in _backgroundSteps)
             {
-                if (step.ExecutionContext.Result.HasValue)
+                // A step that succeeded does not set a Result by default, so a missing
+                // value means the step succeeded and there is nothing to merge.
+                if (!step.ExecutionContext.Result.HasValue)
                 {
-                    result = TaskResultUtil.MergeTaskResults(result, step.ExecutionContext.Result.Value);
+                    continue;
                 }
+
+                // A step explicitly canceled via a `cancel` control step is expected to be canceled,
+                // so a Canceled result must not influence the overall job result. However, if the step
+                // failed (e.g. before the cancellation took effect), that failure should still count.
+                if (_explicitlyCanceledStepIds.Contains(stepId) &&
+                    step.ExecutionContext.Result.Value == TaskResult.Canceled)
+                {
+                    continue;
+                }
+
+                result = TaskResultUtil.MergeTaskResults(result, step.ExecutionContext.Result.Value);
             }
 
             if (result != TaskResult.Succeeded)
@@ -256,6 +277,13 @@ namespace GitHub.Runner.Worker
                 return;
             }
 
+            // Mark these steps as expected-to-be-canceled so their result does not
+            // affect the overall job result.
+            foreach (var id in cancelStepIds)
+            {
+                _explicitlyCanceledStepIds.Add(id);
+            }
+
             var idsToCancel = cancelStepIds
                 .Where(id => _backgroundSteps.ContainsKey(id) && !_backgroundSteps[id].Task.IsCompleted)
                 .ToArray();

src/Test/L0/Worker/BackgroundStepsL0.cs

@@ -372,6 +372,88 @@ namespace GitHub.Runner.Common.Tests.Worker
             }
         }
 
+        [Fact]
+        [Trait("Level", "L0")]
+        [Trait("Category", "Worker")]
+        public async Task CanceledBackgroundStepDoesNotAffectJobResult()
+        {
+            using (TestHostContext hc = CreateTestContext())
+            {
+                // Arrange: a background step that runs until explicitly canceled. When canceled it
+                // reports TaskResult.Canceled, but since the cancellation is expected (driven by a
+                // cancel control step), it must not impact the overall job result.
+                using var stepCts = new CancellationTokenSource();
+
+                var bgStep = CreateStep(hc, TaskResult.Succeeded, "success()", name: "server", contextName: "server", isBackground: true);
+                var bgStepContext = Mock.Get(bgStep.Object.ExecutionContext);
+                bgStepContext.Setup(x => x.CancellationToken).Returns(stepCts.Token);
+                bgStepContext.Setup(x => x.CancelToken()).Callback(() => stepCts.Cancel());
+                bgStep.Setup(x => x.RunAsync()).Returns(async () =>
+                {
+                    await Task.Delay(TimeSpan.FromSeconds(2), stepCts.Token);
+                });
+                bgStep.Setup(x => x.Action).Returns(new GitHub.DistributedTask.Pipelines.ActionStep()
+                {
+                    Name = "server",
+                    Id = Guid.NewGuid(),
+                    ContextName = "server",
+                    Background = true,
+                });
+
+                var cancelStep = CreateCancelStep(hc, "server");
+
+                _ec.Object.Result = null;
+                _ec.Setup(x => x.JobSteps).Returns(new Queue<IStep>(new IStep[]
+                {
+                    bgStep.Object, cancelStep
+                }));
+
+                // Act
+                await _stepsRunner.RunAsync(jobContext: _ec.Object);
+
+                // Assert: the canceled background step reported Canceled, but the job result is unaffected.
+                Assert.Equal(TaskResult.Canceled, bgStep.Object.ExecutionContext.Result);
+                Assert.Equal(TaskResult.Succeeded, _ec.Object.Result ?? TaskResult.Succeeded);
+            }
+        }
+
+        [Fact]
+        [Trait("Level", "L0")]
+        [Trait("Category", "Worker")]
+        public async Task FailedBackgroundStepTargetedByCancelStillAffectsJobResult()
+        {
+            using (TestHostContext hc = CreateTestContext())
+            {
+                // Arrange: a background step that fails (e.g. before the cancel takes effect). Even
+                // though a cancel control step targets it, its Failed result must still propagate to
+                // the overall job result.
+                var bgStep = CreateStep(hc, TaskResult.Failed, "success()", name: "server", contextName: "server", isBackground: true);
+                bgStep.Setup(x => x.RunAsync()).Returns(Task.CompletedTask);
+                bgStep.Setup(x => x.Action).Returns(new GitHub.DistributedTask.Pipelines.ActionStep()
+                {
+                    Name = "server",
+                    Id = Guid.NewGuid(),
+                    ContextName = "server",
+                    Background = true,
+                });
+
+                var cancelStep = CreateCancelStep(hc, "server");
+
+                _ec.Object.Result = null;
+                _ec.Setup(x => x.JobSteps).Returns(new Queue<IStep>(new IStep[]
+                {
+                    bgStep.Object, cancelStep
+                }));
+
+                // Act
+                await _stepsRunner.RunAsync(jobContext: _ec.Object);
+
+                // Assert: the background step failed, so the job result reflects that failure.
+                Assert.Equal(TaskResult.Failed, bgStep.Object.ExecutionContext.Result);
+                Assert.Equal(TaskResult.Failed, _ec.Object.Result);
+            }
+        }
+
         [Fact]
         [Trait("Level", "L0")]
         [Trait("Category", "Worker")]