From f8366c7a7b5ddc924a24ca94722d39fc39724447 Mon Sep 17 00:00:00 2001 From: Lokesh Gopu Date: Mon, 8 Jun 2026 17:18:11 -0400 Subject: [PATCH] Canceled background steps should not impact job result (#4482) --- .../BackgroundStepCoordinator.cs | 36 +++++++- src/Test/L0/Worker/BackgroundStepsL0.cs | 82 +++++++++++++++++++ 2 files changed, 114 insertions(+), 4 deletions(-) diff --git a/src/Runner.Worker/BackgroundStepCoordinator.cs b/src/Runner.Worker/BackgroundStepCoordinator.cs index 0bbbe1ed8..a040903ad 100644 --- a/src/Runner.Worker/BackgroundStepCoordinator.cs +++ b/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 _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 _explicitlyCanceledStepIds = new(); private SemaphoreSlim _backgroundSlotSemaphore = new SemaphoreSlim(DefaultMaxBackgroundSteps); /// @@ -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 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(); diff --git a/src/Test/L0/Worker/BackgroundStepsL0.cs b/src/Test/L0/Worker/BackgroundStepsL0.cs index 9e7827da8..62f98351d 100644 --- a/src/Test/L0/Worker/BackgroundStepsL0.cs +++ b/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(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(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")]