From e3c0a6e1191732c1aebdbdffc047e0f4dd663c8d Mon Sep 17 00:00:00 2001 From: Francesco Renzi Date: Tue, 17 Mar 2026 08:55:27 +0000 Subject: [PATCH] PR Feedback --- src/Runner.Worker/Dap/DapDebugger.cs | 41 ++++++++++++++------------- src/Runner.Worker/Dap/IDapDebugger.cs | 4 +-- src/Runner.Worker/JobRunner.cs | 30 ++++++-------------- src/Runner.Worker/StepsRunner.cs | 26 ++--------------- src/Test/L0/Worker/DapDebuggerL0.cs | 11 ++++--- src/Test/L0/Worker/JobExtensionL0.cs | 6 ++++ src/Test/L0/Worker/StepsRunnerL0.cs | 6 ++++ 7 files changed, 53 insertions(+), 71 deletions(-) diff --git a/src/Runner.Worker/Dap/DapDebugger.cs b/src/Runner.Worker/Dap/DapDebugger.cs index b4d515865..aa2048e7d 100644 --- a/src/Runner.Worker/Dap/DapDebugger.cs +++ b/src/Runner.Worker/Dap/DapDebugger.cs @@ -21,6 +21,7 @@ namespace GitHub.Runner.Worker.Dap private IDapDebugSession _session; private CancellationTokenRegistration? _cancellationRegistration; private volatile bool _started; + private bool _isFirstStep = true; public bool IsActive => _session?.IsActive == true; @@ -78,6 +79,23 @@ namespace GitHub.Runner.Worker.Dap }); } + public async Task OnJobCompletedAsync() + { + if (_session != null && _started) + { + try + { + _session.OnJobCompleted(); + } + catch (Exception ex) + { + Trace.Warning($"DAP OnJobCompleted error: {ex.Message}"); + } + } + + await StopAsync(); + } + public async Task StopAsync() { if (_cancellationRegistration.HasValue) @@ -108,7 +126,7 @@ namespace GitHub.Runner.Worker.Dap _session?.CancelSession(); } - public async Task OnStepStartingAsync(IStep step, IExecutionContext jobContext, bool isFirstStep, CancellationToken cancellationToken) + public async Task OnStepStartingAsync(IStep step, IExecutionContext jobContext, CancellationToken cancellationToken) { if (!IsActive) { @@ -117,7 +135,9 @@ namespace GitHub.Runner.Worker.Dap try { - await _session.OnStepStartingAsync(step, jobContext, isFirstStep, cancellationToken); + bool isFirst = _isFirstStep; + _isFirstStep = false; + await _session.OnStepStartingAsync(step, jobContext, isFirst, cancellationToken); } catch (Exception ex) { @@ -142,23 +162,6 @@ namespace GitHub.Runner.Worker.Dap } } - public void OnJobCompleted() - { - if (!IsActive) - { - return; - } - - try - { - _session.OnJobCompleted(); - } - catch (Exception ex) - { - Trace.Warning($"DAP OnJobCompleted error: {ex.Message}"); - } - } - private int ResolvePort() { var portEnv = Environment.GetEnvironmentVariable(PortEnvironmentVariable); diff --git a/src/Runner.Worker/Dap/IDapDebugger.cs b/src/Runner.Worker/Dap/IDapDebugger.cs index f753b6f7c..45a85e8fc 100644 --- a/src/Runner.Worker/Dap/IDapDebugger.cs +++ b/src/Runner.Worker/Dap/IDapDebugger.cs @@ -12,8 +12,8 @@ namespace GitHub.Runner.Worker.Dap Task WaitUntilReadyAsync(CancellationToken cancellationToken); Task StopAsync(); void CancelSession(); - Task OnStepStartingAsync(IStep step, IExecutionContext jobContext, bool isFirstStep, CancellationToken cancellationToken); + Task OnStepStartingAsync(IStep step, IExecutionContext jobContext, CancellationToken cancellationToken); void OnStepCompleted(IStep step); - void OnJobCompleted(); + Task OnJobCompletedAsync(); } } diff --git a/src/Runner.Worker/JobRunner.cs b/src/Runner.Worker/JobRunner.cs index 4d7ac04a5..d5ecdeffd 100644 --- a/src/Runner.Worker/JobRunner.cs +++ b/src/Runner.Worker/JobRunner.cs @@ -193,11 +193,8 @@ namespace GitHub.Runner.Worker catch (Exception ex) { Trace.Error($"Failed to start DAP debugger: {ex.Message}"); - if (dapDebugger != null) - { - try { await dapDebugger.StopAsync(); } catch { } - } - dapDebugger = null; + jobContext.Error("Failed to start debugger."); + return await CompleteJobAsync(server, jobContext, message, TaskResult.Failed); } } @@ -254,24 +251,18 @@ namespace GitHub.Runner.Worker catch (OperationCanceledException) when (jobRequestCancellationToken.IsCancellationRequested) { Trace.Info("Job was cancelled before debugger client connected."); - try { await dapDebugger.StopAsync(); } catch { } - dapDebugger = null; + jobContext.Error("Job was cancelled before debugger client connected."); return await CompleteJobAsync(server, jobContext, message, TaskResult.Canceled); } catch (Exception ex) { Trace.Error($"DAP debugger failed to become ready: {ex.Message}"); - try { await dapDebugger.StopAsync(); } catch { } - dapDebugger = null; - } - } - // If debugging was requested but the debugger is not available, fail the job - if (jobContext.Global.EnableDebugger && dapDebugger == null) - { - var errorMessage = "The debugger failed to start or no debugger client connected in time."; - jobContext.Error(errorMessage); - return await CompleteJobAsync(server, jobContext, message, TaskResult.Failed); + // If debugging was requested but the debugger is not available, fail the job + var errorMessage = "The debugger failed to start or no debugger client connected in time."; + jobContext.Error(errorMessage); + return await CompleteJobAsync(server, jobContext, message, TaskResult.Failed); + } } // Run all job steps @@ -314,10 +305,7 @@ namespace GitHub.Runner.Worker runnerShutdownRegistration = null; } - if (dapDebugger != null) - { - await dapDebugger.StopAsync(); - } + await dapDebugger?.OnJobCompletedAsync(); await ShutdownQueue(throwOnFailure: false); } diff --git a/src/Runner.Worker/StepsRunner.cs b/src/Runner.Worker/StepsRunner.cs index c7309be82..a10901200 100644 --- a/src/Runner.Worker/StepsRunner.cs +++ b/src/Runner.Worker/StepsRunner.cs @@ -51,16 +51,7 @@ namespace GitHub.Runner.Worker jobContext.JobContext.Status = (jobContext.Result ?? TaskResult.Succeeded).ToActionResult(); var scopeInputs = new Dictionary(StringComparer.OrdinalIgnoreCase); bool checkPostJobActions = false; - IDapDebugger dapDebugger = null; - try - { - dapDebugger = HostContext.GetService(); - } - catch - { - // Debugger not available — continue without debugging - } - bool isFirstStep = true; + var dapDebugger = HostContext.GetService(); while (jobContext.JobSteps.Count > 0 || !checkPostJobActions) { if (jobContext.JobSteps.Count == 0 && !checkPostJobActions) @@ -238,20 +229,13 @@ namespace GitHub.Runner.Worker else { // Pause for DAP debugger before step execution - if (dapDebugger != null) - { - await dapDebugger.OnStepStartingAsync(step, jobContext, isFirstStep, jobContext.CancellationToken); - isFirstStep = false; - } + await dapDebugger?.OnStepStartingAsync(step, jobContext, jobContext.CancellationToken); // Run the step await RunStepAsync(step, jobContext.CancellationToken); CompleteStep(step); - if (dapDebugger != null) - { - dapDebugger.OnStepCompleted(step); - } + dapDebugger?.OnStepCompleted(step); } } finally @@ -279,10 +263,6 @@ namespace GitHub.Runner.Worker Trace.Info($"Current state: job state = '{jobContext.Result}'"); } - if (dapDebugger != null) - { - dapDebugger.OnJobCompleted(); - } } private async Task RunStepAsync(IStep step, CancellationToken jobCancellationToken) diff --git a/src/Test/L0/Worker/DapDebuggerL0.cs b/src/Test/L0/Worker/DapDebuggerL0.cs index 4838aaf46..ddf6f0153 100644 --- a/src/Test/L0/Worker/DapDebuggerL0.cs +++ b/src/Test/L0/Worker/DapDebuggerL0.cs @@ -273,7 +273,7 @@ namespace GitHub.Runner.Common.Tests.Worker var mockStep = new Mock(); var mockJobContext = new Mock(); - await _debugger.OnStepStartingAsync(mockStep.Object, mockJobContext.Object, true, CancellationToken.None); + await _debugger.OnStepStartingAsync(mockStep.Object, mockJobContext.Object, CancellationToken.None); mockSession.Verify(x => x.OnStepStartingAsync(mockStep.Object, mockJobContext.Object, true, CancellationToken.None), Times.Once); @@ -306,7 +306,7 @@ namespace GitHub.Runner.Common.Tests.Worker var mockStep = new Mock(); var mockJobContext = new Mock(); - await _debugger.OnStepStartingAsync(mockStep.Object, mockJobContext.Object, true, CancellationToken.None); + await _debugger.OnStepStartingAsync(mockStep.Object, mockJobContext.Object, CancellationToken.None); mockSession.Verify(x => x.OnStepStartingAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); @@ -367,11 +367,10 @@ namespace GitHub.Runner.Common.Tests.Worker var cts = new CancellationTokenSource(); await _debugger.StartAsync(cts.Token); - _debugger.OnJobCompleted(); + await _debugger.OnJobCompletedAsync(); mockSession.Verify(x => x.OnJobCompleted(), Times.Once); - - await _debugger.StopAsync(); + mockServer.Verify(x => x.StopAsync(), Times.Once); } } @@ -403,7 +402,7 @@ namespace GitHub.Runner.Common.Tests.Worker var mockJobContext = new Mock(); // Should not throw - await _debugger.OnStepStartingAsync(mockStep.Object, mockJobContext.Object, true, CancellationToken.None); + await _debugger.OnStepStartingAsync(mockStep.Object, mockJobContext.Object, CancellationToken.None); await _debugger.StopAsync(); } diff --git a/src/Test/L0/Worker/JobExtensionL0.cs b/src/Test/L0/Worker/JobExtensionL0.cs index 60814998e..34413552c 100644 --- a/src/Test/L0/Worker/JobExtensionL0.cs +++ b/src/Test/L0/Worker/JobExtensionL0.cs @@ -8,6 +8,7 @@ using GitHub.DistributedTask.ObjectTemplating.Tokens; using GitHub.DistributedTask.Pipelines.ObjectTemplating; using GitHub.DistributedTask.WebApi; using GitHub.Runner.Worker; +using GitHub.Runner.Worker.Dap; using Moq; using Xunit; using Pipelines = GitHub.DistributedTask.Pipelines; @@ -547,6 +548,11 @@ namespace GitHub.Runner.Common.Tests.Worker var _stepsRunner = new StepsRunner(); _stepsRunner.Initialize(hc); + + var mockDapDebugger = new Mock(); + mockDapDebugger.Setup(x => x.IsActive).Returns(false); + hc.SetSingleton(mockDapDebugger.Object); + await _stepsRunner.RunAsync(_jobEc); Assert.Equal("Create custom image", snapshotStep.DisplayName); diff --git a/src/Test/L0/Worker/StepsRunnerL0.cs b/src/Test/L0/Worker/StepsRunnerL0.cs index a22dc618f..e9e99d82c 100644 --- a/src/Test/L0/Worker/StepsRunnerL0.cs +++ b/src/Test/L0/Worker/StepsRunnerL0.cs @@ -12,6 +12,7 @@ using GitHub.DistributedTask.ObjectTemplating.Tokens; using GitHub.DistributedTask.WebApi; using GitHub.Runner.Common.Util; using GitHub.Runner.Worker; +using GitHub.Runner.Worker.Dap; namespace GitHub.Runner.Common.Tests.Worker { @@ -61,6 +62,11 @@ namespace GitHub.Runner.Common.Tests.Worker _stepsRunner = new StepsRunner(); _stepsRunner.Initialize(hc); + + var mockDapDebugger = new Mock(); + mockDapDebugger.Setup(x => x.IsActive).Returns(false); + hc.SetSingleton(mockDapDebugger.Object); + return hc; }