diff --git a/src/Runner.Common/Util/UnixUtil.cs b/src/Runner.Common/Util/UnixUtil.cs index 3500b185648..206ae1a63a1 100644 --- a/src/Runner.Common/Util/UnixUtil.cs +++ b/src/Runner.Common/Util/UnixUtil.cs @@ -42,21 +42,24 @@ public async Task ExecAsync(string workingDirectory, string toolName, string arg string toolPath = WhichUtil.Which(toolName, trace: Trace); Trace.Info($"Running {toolPath} {argLine}"); - var processInvoker = HostContext.CreateService(); - processInvoker.OutputDataReceived += OnOutputDataReceived; - processInvoker.ErrorDataReceived += OnErrorDataReceived; - - try + // Dispose invoker per call to release process/CTS resources promptly. + using (var processInvoker = HostContext.CreateService()) { - using (var cs = new CancellationTokenSource(TimeSpan.FromSeconds(45))) + processInvoker.OutputDataReceived += OnOutputDataReceived; + processInvoker.ErrorDataReceived += OnErrorDataReceived; + + try { - await processInvoker.ExecuteAsync(workingDirectory, toolPath, argLine, null, true, cs.Token); + using (var cs = new CancellationTokenSource(TimeSpan.FromSeconds(45))) + { + await processInvoker.ExecuteAsync(workingDirectory, toolPath, argLine, null, true, cs.Token); + } + } + finally + { + processInvoker.OutputDataReceived -= OnOutputDataReceived; + processInvoker.ErrorDataReceived -= OnErrorDataReceived; } - } - finally - { - processInvoker.OutputDataReceived -= OnOutputDataReceived; - processInvoker.ErrorDataReceived -= OnErrorDataReceived; } } diff --git a/src/Runner.Worker/Container/DockerCommandManager.cs b/src/Runner.Worker/Container/DockerCommandManager.cs index 41b914a5ee0..91e533755fb 100644 --- a/src/Runner.Worker/Container/DockerCommandManager.cs +++ b/src/Runner.Worker/Container/DockerCommandManager.cs @@ -309,45 +309,48 @@ public async Task DockerExec(IExecutionContext context, string containerId, { ArgUtil.NotNull(output, nameof(output)); + if (!Constants.Runner.Platform.Equals(Constants.OSPlatform.Linux)) + { + throw new NotSupportedException("Container operations are only supported on Linux runners"); + } + string arg = $"exec {options} {containerId} {command}".Trim(); context.Command($"{DockerPath} {arg}"); object outputLock = new(); - var processInvoker = HostContext.CreateService(); - processInvoker.OutputDataReceived += delegate (object sender, ProcessDataReceivedEventArgs message) + using (var processInvoker = HostContext.CreateService()) { - if (!string.IsNullOrEmpty(message.Data)) + processInvoker.OutputDataReceived += delegate (object sender, ProcessDataReceivedEventArgs message) { - lock (outputLock) + if (!string.IsNullOrEmpty(message.Data)) { - output.Add(message.Data); + lock (outputLock) + { + output.Add(message.Data); + } } - } - }; + }; - processInvoker.ErrorDataReceived += delegate (object sender, ProcessDataReceivedEventArgs message) - { - if (!string.IsNullOrEmpty(message.Data)) + processInvoker.ErrorDataReceived += delegate (object sender, ProcessDataReceivedEventArgs message) { - lock (outputLock) + if (!string.IsNullOrEmpty(message.Data)) { - output.Add(message.Data); + lock (outputLock) + { + output.Add(message.Data); + } } - } - }; - - if (!Constants.Runner.Platform.Equals(Constants.OSPlatform.Linux)) - { - throw new NotSupportedException("Container operations are only supported on Linux runners"); + }; + + return await processInvoker.ExecuteAsync( + workingDirectory: HostContext.GetDirectory(WellKnownDirectory.Work), + fileName: DockerPath, + arguments: arg, + environment: null, + requireExitCodeZero: false, + outputEncoding: null, + cancellationToken: CancellationToken.None); } - return await processInvoker.ExecuteAsync( - workingDirectory: HostContext.GetDirectory(WellKnownDirectory.Work), - fileName: DockerPath, - arguments: arg, - environment: null, - requireExitCodeZero: false, - outputEncoding: null, - cancellationToken: CancellationToken.None); } public async Task> DockerInspect(IExecutionContext context, string dockerObject, string options) @@ -361,7 +364,7 @@ public async Task> DockerPort(IExecutionContext context, strin return DockerUtil.ParseDockerPort(portMappingLines); } - public Task DockerLogin(IExecutionContext context, string configFileDirectory, string registry, string username, string password) + public async Task DockerLogin(IExecutionContext context, string configFileDirectory, string registry, string username, string password) { string args = $"--config {configFileDirectory} login {registry} -u {username} --password-stdin"; context.Command($"{DockerPath} {args}"); @@ -369,18 +372,19 @@ public Task DockerLogin(IExecutionContext context, string configFileDirecto var input = Channel.CreateBounded(new BoundedChannelOptions(1) { SingleReader = true, SingleWriter = true }); input.Writer.TryWrite(password); - var processInvoker = HostContext.CreateService(); - - return processInvoker.ExecuteAsync( - workingDirectory: context.GetGitHubContext("workspace"), - fileName: DockerPath, - arguments: args, - environment: null, - requireExitCodeZero: false, - outputEncoding: null, - killProcessOnCancel: false, - redirectStandardIn: input, - cancellationToken: context.CancellationToken); + using (var processInvoker = HostContext.CreateService()) + { + return await processInvoker.ExecuteAsync( + workingDirectory: context.GetGitHubContext("workspace"), + fileName: DockerPath, + arguments: args, + environment: null, + requireExitCodeZero: false, + outputEncoding: null, + killProcessOnCancel: false, + redirectStandardIn: input, + cancellationToken: context.CancellationToken); + } } private Task ExecuteDockerCommandAsync(IExecutionContext context, string command, string options, CancellationToken cancellationToken = default(CancellationToken)) @@ -390,59 +394,64 @@ public Task DockerLogin(IExecutionContext context, string configFileDirecto private async Task ExecuteDockerCommandAsync(IExecutionContext context, string command, string options, IDictionary environment, EventHandler stdoutDataReceived, EventHandler stderrDataReceived, CancellationToken cancellationToken = default(CancellationToken)) { + if (!Constants.Runner.Platform.Equals(Constants.OSPlatform.Linux)) + { + throw new NotSupportedException("Container operations are only supported on Linux runners"); + } + string arg = $"{command} {options}".Trim(); context.Command($"{DockerPath} {arg}"); - var processInvoker = HostContext.CreateService(); - processInvoker.OutputDataReceived += stdoutDataReceived; - processInvoker.ErrorDataReceived += stderrDataReceived; - - - if (!Constants.Runner.Platform.Equals(Constants.OSPlatform.Linux)) + using (var processInvoker = HostContext.CreateService()) { - throw new NotSupportedException("Container operations are only supported on Linux runners"); + processInvoker.OutputDataReceived += stdoutDataReceived; + processInvoker.ErrorDataReceived += stderrDataReceived; + + return await processInvoker.ExecuteAsync( + workingDirectory: context.GetGitHubContext("workspace"), + fileName: DockerPath, + arguments: arg, + environment: environment, + requireExitCodeZero: false, + outputEncoding: null, + killProcessOnCancel: false, + cancellationToken: cancellationToken); } - return await processInvoker.ExecuteAsync( - workingDirectory: context.GetGitHubContext("workspace"), - fileName: DockerPath, - arguments: arg, - environment: environment, - requireExitCodeZero: false, - outputEncoding: null, - killProcessOnCancel: false, - cancellationToken: cancellationToken); } private async Task ExecuteDockerCommandAsync(IExecutionContext context, string command, string options, string workingDirectory, CancellationToken cancellationToken = default(CancellationToken)) { + if (!Constants.Runner.Platform.Equals(Constants.OSPlatform.Linux)) + { + throw new NotSupportedException("Container operations are only supported on Linux runners"); + } + string arg = $"{command} {options}".Trim(); context.Command($"{DockerPath} {arg}"); - var processInvoker = HostContext.CreateService(); - processInvoker.OutputDataReceived += delegate (object sender, ProcessDataReceivedEventArgs message) - { - context.Output(message.Data); - }; - - processInvoker.ErrorDataReceived += delegate (object sender, ProcessDataReceivedEventArgs message) + using (var processInvoker = HostContext.CreateService()) { - context.Output(message.Data); - }; + processInvoker.OutputDataReceived += delegate (object sender, ProcessDataReceivedEventArgs message) + { + context.Output(message.Data); + }; - if (!Constants.Runner.Platform.Equals(Constants.OSPlatform.Linux)) - { - throw new NotSupportedException("Container operations are only supported on Linux runners"); + processInvoker.ErrorDataReceived += delegate (object sender, ProcessDataReceivedEventArgs message) + { + context.Output(message.Data); + }; + + return await processInvoker.ExecuteAsync( + workingDirectory: workingDirectory ?? context.GetGitHubContext("workspace"), + fileName: DockerPath, + arguments: arg, + environment: null, + requireExitCodeZero: false, + outputEncoding: null, + killProcessOnCancel: false, + redirectStandardIn: null, + cancellationToken: cancellationToken); } - return await processInvoker.ExecuteAsync( - workingDirectory: workingDirectory ?? context.GetGitHubContext("workspace"), - fileName: DockerPath, - arguments: arg, - environment: null, - requireExitCodeZero: false, - outputEncoding: null, - killProcessOnCancel: false, - redirectStandardIn: null, - cancellationToken: cancellationToken); } private async Task> ExecuteDockerCommandAsync(IExecutionContext context, string command, string options) @@ -451,32 +460,34 @@ private async Task> ExecuteDockerCommandAsync(IExecutionContext con context.Command($"{DockerPath} {arg}"); List output = new(); - var processInvoker = HostContext.CreateService(); - processInvoker.OutputDataReceived += delegate (object sender, ProcessDataReceivedEventArgs message) + using (var processInvoker = HostContext.CreateService()) { - if (!string.IsNullOrEmpty(message.Data)) + processInvoker.OutputDataReceived += delegate (object sender, ProcessDataReceivedEventArgs message) { - output.Add(message.Data); - context.Output(message.Data); - } - }; + if (!string.IsNullOrEmpty(message.Data)) + { + output.Add(message.Data); + context.Output(message.Data); + } + }; - processInvoker.ErrorDataReceived += delegate (object sender, ProcessDataReceivedEventArgs message) - { - if (!string.IsNullOrEmpty(message.Data)) + processInvoker.ErrorDataReceived += delegate (object sender, ProcessDataReceivedEventArgs message) { - context.Output(message.Data); - } - }; - - await processInvoker.ExecuteAsync( - workingDirectory: context.GetGitHubContext("workspace"), - fileName: DockerPath, - arguments: arg, - environment: null, - requireExitCodeZero: true, - outputEncoding: null, - cancellationToken: CancellationToken.None); + if (!string.IsNullOrEmpty(message.Data)) + { + context.Output(message.Data); + } + }; + + await processInvoker.ExecuteAsync( + workingDirectory: context.GetGitHubContext("workspace"), + fileName: DockerPath, + arguments: arg, + environment: null, + requireExitCodeZero: true, + outputEncoding: null, + cancellationToken: CancellationToken.None); + } return output; } diff --git a/src/Runner.Worker/ContainerOperationProvider.cs b/src/Runner.Worker/ContainerOperationProvider.cs index c5cccb77ef0..900d87a4d74 100644 --- a/src/Runner.Worker/ContainerOperationProvider.cs +++ b/src/Runner.Worker/ContainerOperationProvider.cs @@ -362,37 +362,39 @@ private async Task> ExecuteCommandAsync(IExecutionContext context, List outputs = new(); object outputLock = new(); - var processInvoker = HostContext.CreateService(); - processInvoker.OutputDataReceived += delegate (object sender, ProcessDataReceivedEventArgs message) + using (var processInvoker = HostContext.CreateService()) { - if (!string.IsNullOrEmpty(message.Data)) + processInvoker.OutputDataReceived += delegate (object sender, ProcessDataReceivedEventArgs message) { - lock (outputLock) + if (!string.IsNullOrEmpty(message.Data)) { - outputs.Add(message.Data); + lock (outputLock) + { + outputs.Add(message.Data); + } } - } - }; + }; - processInvoker.ErrorDataReceived += delegate (object sender, ProcessDataReceivedEventArgs message) - { - if (!string.IsNullOrEmpty(message.Data)) + processInvoker.ErrorDataReceived += delegate (object sender, ProcessDataReceivedEventArgs message) { - lock (outputLock) + if (!string.IsNullOrEmpty(message.Data)) { - outputs.Add(message.Data); + lock (outputLock) + { + outputs.Add(message.Data); + } } - } - }; - - await processInvoker.ExecuteAsync( - workingDirectory: HostContext.GetDirectory(WellKnownDirectory.Work), - fileName: command, - arguments: arg, - environment: null, - requireExitCodeZero: true, - outputEncoding: null, - cancellationToken: CancellationToken.None); + }; + + await processInvoker.ExecuteAsync( + workingDirectory: HostContext.GetDirectory(WellKnownDirectory.Work), + fileName: command, + arguments: arg, + environment: null, + requireExitCodeZero: true, + outputEncoding: null, + cancellationToken: CancellationToken.None); + } foreach (var outputLine in outputs) { diff --git a/src/Test/L0/ProcessInvokerDisposalL0.cs b/src/Test/L0/ProcessInvokerDisposalL0.cs new file mode 100644 index 00000000000..50bdcff8558 --- /dev/null +++ b/src/Test/L0/ProcessInvokerDisposalL0.cs @@ -0,0 +1,105 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using System.Threading; +using System.Threading.Channels; +using System.Threading.Tasks; +using GitHub.Runner.Sdk; +using Xunit; + +namespace GitHub.Runner.Common.Tests +{ + // Verifies CreateService() call sites dispose per invocation. + public sealed class ProcessInvokerDisposalL0 + { + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Common")] + public void Fix_CreateServiceInUsingBlock_CallsDisposeExactlyOnce() + { + using (TestHostContext hc = new(this)) + { + var tracker = new DisposalTrackingProcessInvoker(); + hc.EnqueueInstance(tracker); + + using (var processInvoker = hc.CreateService()) + { + _ = processInvoker; + } + + Assert.Equal(1, tracker.DisposeCount); + } + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Common")] + public async Task Fix_DisposeIsCalledEvenIfExecuteThrows() + { + using (TestHostContext hc = new(this)) + { + var tracker = new DisposalTrackingProcessInvoker { ThrowOnExecute = true }; + hc.EnqueueInstance(tracker); + + await Assert.ThrowsAsync(async () => + { + using (var processInvoker = hc.CreateService()) + { + await processInvoker.ExecuteAsync("", "tool", "", null, CancellationToken.None); + } + }); + + Assert.Equal(1, tracker.DisposeCount); + } + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Common")] + public void Fix_ProcessInvokerWrapperDispose_ReleasesInnerProcessInvoker() + { + using (TestHostContext hc = new(this)) + { + var wrapper = new ProcessInvokerWrapper(); + wrapper.Initialize(hc); + + wrapper.Dispose(); + wrapper.Dispose(); + } + } + } + + // Minimal invoker that records Dispose() calls. + internal sealed class DisposalTrackingProcessInvoker : IProcessInvoker + { + public int DisposeCount { get; private set; } + public bool ThrowOnExecute { get; set; } + + public void Initialize(IHostContext hostContext) { } + + public event EventHandler OutputDataReceived; + public event EventHandler ErrorDataReceived; + + private Task FailOrReturn() => + ThrowOnExecute ? Task.FromException(new InvalidOperationException("simulated failure")) : Task.FromResult(0); + + // Silence CS0067 "event never used" warnings by nominally attaching/removing. + private void _touch() + { + OutputDataReceived?.Invoke(this, null); + ErrorDataReceived?.Invoke(this, null); + } + + public Task ExecuteAsync(string workingDirectory, string fileName, string arguments, IDictionary environment, CancellationToken cancellationToken) => FailOrReturn(); + public Task ExecuteAsync(string workingDirectory, string fileName, string arguments, IDictionary environment, bool requireExitCodeZero, CancellationToken cancellationToken) => FailOrReturn(); + public Task ExecuteAsync(string workingDirectory, string fileName, string arguments, IDictionary environment, bool requireExitCodeZero, Encoding outputEncoding, CancellationToken cancellationToken) => FailOrReturn(); + public Task ExecuteAsync(string workingDirectory, string fileName, string arguments, IDictionary environment, bool requireExitCodeZero, Encoding outputEncoding, bool killProcessOnCancel, CancellationToken cancellationToken) => FailOrReturn(); + public Task ExecuteAsync(string workingDirectory, string fileName, string arguments, IDictionary environment, bool requireExitCodeZero, Encoding outputEncoding, bool killProcessOnCancel, Channel redirectStandardIn, CancellationToken cancellationToken) => FailOrReturn(); + public Task ExecuteAsync(string workingDirectory, string fileName, string arguments, IDictionary environment, bool requireExitCodeZero, Encoding outputEncoding, bool killProcessOnCancel, Channel redirectStandardIn, bool inheritConsoleHandler, CancellationToken cancellationToken) => FailOrReturn(); + public Task ExecuteAsync(string workingDirectory, string fileName, string arguments, IDictionary environment, bool requireExitCodeZero, Encoding outputEncoding, bool killProcessOnCancel, Channel redirectStandardIn, bool inheritConsoleHandler, bool keepStandardInOpen, CancellationToken cancellationToken) => FailOrReturn(); + public Task ExecuteAsync(string workingDirectory, string fileName, string arguments, IDictionary environment, bool requireExitCodeZero, Encoding outputEncoding, bool killProcessOnCancel, Channel redirectStandardIn, bool inheritConsoleHandler, bool keepStandardInOpen, bool highPriorityProcess, CancellationToken cancellationToken) => FailOrReturn(); + + public void Dispose() => DisposeCount++; + } +} diff --git a/src/Test/L0/Util/UnixUtilL0.cs b/src/Test/L0/Util/UnixUtilL0.cs new file mode 100644 index 00000000000..d755c0a408b --- /dev/null +++ b/src/Test/L0/Util/UnixUtilL0.cs @@ -0,0 +1,60 @@ +using System; +using System.IO; +using System.Threading.Tasks; +using GitHub.Runner.Common.Util; +using GitHub.Runner.Sdk; +using Xunit; + +namespace GitHub.Runner.Common.Tests.Util +{ + // Verifies UnixUtil disposes its per-call process invoker. + public sealed class UnixUtilL0 + { +#if !OS_WINDOWS + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Common")] + public async Task ExecAsync_DisposesIProcessInvoker_OnSuccess() + { + using (TestHostContext hc = new(this)) + { + var tracker = new DisposalTrackingProcessInvoker(); + hc.EnqueueInstance(tracker); + + var unix = new UnixUtil(); + unix.Initialize(hc); + + // Use full path to bypass PATH lookup variance. + string toolPath = File.Exists("/bin/true") ? "/bin/true" : "/usr/bin/true"; + Assert.True(File.Exists(toolPath), $"expected a no-op tool at {toolPath}"); + + await unix.ExecAsync(workingDirectory: ".", toolName: toolPath, argLine: ""); + + Assert.Equal(1, tracker.DisposeCount); + } + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Common")] + public async Task ExecAsync_DisposesIProcessInvoker_EvenWhenProcessThrows() + { + using (TestHostContext hc = new(this)) + { + var tracker = new DisposalTrackingProcessInvoker { ThrowOnExecute = true }; + hc.EnqueueInstance(tracker); + + var unix = new UnixUtil(); + unix.Initialize(hc); + + string toolPath = File.Exists("/bin/true") ? "/bin/true" : "/usr/bin/true"; + + await Assert.ThrowsAsync(() => + unix.ExecAsync(workingDirectory: ".", toolName: toolPath, argLine: "")); + + Assert.Equal(1, tracker.DisposeCount); + } + } +#endif + } +}