diff --git a/core/Azure.Mcp.Core/src/Services/ProcessExecution/ExternalProcessService.cs b/core/Azure.Mcp.Core/src/Services/ProcessExecution/ExternalProcessService.cs index 062d4e0641..eb93788bc1 100644 --- a/core/Azure.Mcp.Core/src/Services/ProcessExecution/ExternalProcessService.cs +++ b/core/Azure.Mcp.Core/src/Services/ProcessExecution/ExternalProcessService.cs @@ -3,153 +3,730 @@ using System.Diagnostics; using System.Text; +using System.Text.Json; using System.Text.Json.Serialization; +using Microsoft.Extensions.Logging; +using static Azure.Mcp.Core.Services.ProcessExecution.ProcessExtensions; namespace Azure.Mcp.Core.Services.ProcessExecution; -public class ExternalProcessService : IExternalProcessService +/// +/// Executes external processes and captures their output in a timeout and cancellation aware way. +/// +public class ExternalProcessService(ILogger logger) : IExternalProcessService { - private readonly Dictionary environmentVariables = []; - - /// + /// + /// Executes an external process and captures its stdout and stderr. + /// + /// The name or path of the executable to run. In the case of a bare executable name, + /// the operating system resolves the executable using its standard search rules (including directories + /// listed in the PATH environment variable). + /// Command-line arguments to pass to the process. + /// + /// Optional environment variables. If null, the process inherits the parent environment. + /// + /// + /// Total timeout for the operation (process execution + exit-wait + stdout/stderr drain). + /// Must be greater than zero. + /// + /// + /// External cancellation token. When triggered, the process is terminated on a best-effort basis + /// and is rethrown. + /// + /// + /// A containing the exit code, stdout, stderr, and the full command. + /// + /// + /// Thrown when is less than or equal to zero. + /// + /// + /// Thrown when is null or empty. + /// + /// + /// Thrown when the process fails to start. + /// + /// + /// Thrown when the operation (process execution and stream draining) exceeds . + /// + /// + /// Thrown when is triggered before the operation completes. + /// + /// + /// Timeout vs. cancellation: + /// + /// Timeout and cancellation are handled independently: + /// + /// + /// + /// + /// is enforced by wrapping the combined + /// (exit-wait + stdout + stderr) task in . + /// A timeout always results in . + /// If the timeout occurs before the process has exited, a best-effort attempt is made to + /// terminate the process (and its tree) before throwing. + /// + /// + /// + /// + /// represents external caller cancellation. When cancellation is + /// signaled, and the process has not yet exited, a best-effort attempt is made to terminate the process + /// (and its tree) before rethrowing the original . + /// + /// + /// + /// + /// This separation avoids the common ambiguity where timeouts are implemented by canceling the caller’s token. + /// Here: always means timeout; + /// always means external cancellation. + /// + /// + /// + /// Diagnostic information on exceptions:
+ /// When a timeout or external cancellation occurs, this method attaches structured diagnostic + /// information to the thrown or + /// via the exception’s Data dictionary. This + /// information does not alter exception semantics or stack traces, and exists solely to aid + /// debugging. The following fields are always included: + /// + /// + /// + /// "ProcessName" — the process name if available, or "<unknown>". + /// "ProcessId" — the process ID if available, or -1. + /// "ProcessExitStatus" — one of Exited, NotExited, or Indeterminate, + /// indicating the observed exit state of the process "before" any kill attempt. + /// + /// + /// + /// Additional diagnostic fields are included only when relevant: + /// + /// + /// + /// + /// + /// "ProcessKillException" — present only when the process was observed to be + /// NotExited and a best-effort attempt to terminate it + /// (via ) failed. The value is the caught exception. + /// + /// + /// + /// + /// "ProcessExitCheckException" — present only when the process exit state could not be + /// determined because itself threw during the exit check. This + /// captures the original exception that made the exit status Indeterminate. + /// + /// + /// + /// + /// + /// These fields are useful for diagnosing cases such as processes failing to terminate, OS-level handle + /// failures, corrupted process state, or unexpected platform behavior. All diagnostic data is attached + /// without modifying the original exception type, message, stack trace, or cancellation semantics. + /// + /// + /// + /// The method does not interpret success or failure using . + /// The exit code is included in the , and the caller decides how to interpret it. + /// + /// + /// public async Task ExecuteAsync( - string executablePath, + string fileName, string arguments, - int timeoutSeconds = 300, - IEnumerable? customPaths = null, + IDictionary? environmentVariables = null, + int operationTimeoutSeconds = 300, CancellationToken cancellationToken = default) { - ArgumentException.ThrowIfNullOrEmpty(executablePath); + var operationTimeout = ValidateTimeout(operationTimeoutSeconds); + + using Process process = CreateProcess(fileName, arguments, environmentVariables); + using ProcessStreamReader stdoutReader = new(process, isErrorStream: false, logger); + using ProcessStreamReader stderrReader = new(process, isErrorStream: true, logger); + + if (!process.Start()) + { + throw new InvalidOperationException($"Failed to start process: {fileName}"); + } + + Task stdoutTask = stdoutReader.ReadToEndAsync(); + Task stderrTask = stderrReader.ReadToEndAsync(); + Task exitTask = process.WaitForExitAsync(cancellationToken); + + Task operation = Task.WhenAll(exitTask, stdoutTask, stderrTask); + + try + { + await operation.WaitAsync(operationTimeout, cancellationToken).ConfigureAwait(false); + } + catch (TimeoutException) + { + // Timeout may be thrown either: + // - Case A: before the process had exited, or + // - Case B: after the process had already exited, but before streams were fully drained. + throw HandleTimeout(process, operationTimeout, fileName, arguments); + } + catch (OperationCanceledException oce) when (cancellationToken.IsCancellationRequested) + { + // Cancellation was explicitly requested by the caller (not a timeout). + // OCE may be thrown either: + // - Case A: by WaitForExitAsync while the process is still running, or + // - Case B: by WaitAsync after the process has already exited. + HandleCancellation(process, fileName, arguments, oce); + // 'throw;' here preserves the original stack trace from where the OCE was first thrown, + // inside WaitAsync or WaitForExitAsync not from this catch block. + throw; + } + + // The earlier await on Task.WhenAll(...) guarantees that stdoutTask and stderrTask have already run + // to completion. The .Result simply retrieves their already-computed output without any blocking. + var stdout = stdoutTask.Result.TrimEnd(); + var stderr = stderrTask.Result.TrimEnd(); + + // Normal completion: the process has exited, and stdout/stderr have fully drained. + return new ProcessResult( + process.ExitCode, + stdout, + stderr, + $"{fileName} {arguments}"); + + // The `using` declarations at the top ensure that both ProcessStreamReader and Process are disposed on + // every exit path—normal completion, timeout, or cancellation — unsubscribing handlers and releasing OS + // resources. + } - if (!File.Exists(executablePath)) + public JsonElement ParseJsonOutput(ProcessResult result) + { + if (result.ExitCode != 0) { - throw new FileNotFoundException($"Executable not found at path: {executablePath}"); + var error = new ParseError( + result.ExitCode, + result.Error, + result.Command); + + return JsonSerializer.SerializeToElement( + error, + ServicesJsonContext.Default.ParseError); } - var processStartInfo = new ProcessStartInfo + try { - FileName = executablePath, + using var jsonDocument = JsonDocument.Parse(result.Output); + return jsonDocument.RootElement.Clone(); + } + catch + { + var fallback = new ParseOutput(result.Output); + return JsonSerializer.SerializeToElement( + fallback, + ServicesJsonContext.Default.ParseOutput); + } + } + + internal record ParseError( + int ExitCode, + string Error, + string Command); + + internal record ParseOutput( + [property: JsonPropertyName("output")] + string Output); + + private static TimeSpan ValidateTimeout(int operationTimeoutSeconds) + { + if (operationTimeoutSeconds <= 0) + { + throw new ArgumentOutOfRangeException(nameof(operationTimeoutSeconds), "Timeout must be a positive number of seconds."); + } + return TimeSpan.FromSeconds(operationTimeoutSeconds); + } + + /// + /// Creates and configures a for execution with redirected stdout/stderr/stdin and + /// applied environment variables. + /// + private static Process CreateProcess(string fileName, string arguments, IDictionary? environmentVariables) + { + ArgumentException.ThrowIfNullOrEmpty(fileName); + + var startInfo = new ProcessStartInfo + { + FileName = fileName, Arguments = arguments, + RedirectStandardOutput = true, RedirectStandardError = true, RedirectStandardInput = true, + UseShellExecute = false, CreateNoWindow = true, StandardOutputEncoding = Encoding.UTF8, StandardErrorEncoding = Encoding.UTF8 }; - foreach (var keyValuePair in environmentVariables) + if (environmentVariables is not null) { - processStartInfo.EnvironmentVariables[keyValuePair.Key] = keyValuePair.Value; + foreach (var pair in environmentVariables) + { + startInfo.EnvironmentVariables[pair.Key] = pair.Value; + } } - using var outputWaitHandle = new AutoResetEvent(false); - using var errorWaitHandle = new AutoResetEvent(false); - using var process = new Process { StartInfo = processStartInfo }; - - var outputBuilder = new StringBuilder(); - var errorBuilder = new StringBuilder(); - - process.OutputDataReceived += (sender, e) => + return new Process { - if (e.Data == null) - outputWaitHandle.Set(); - else - outputBuilder.AppendLine(e.Data); + StartInfo = startInfo, + EnableRaisingEvents = true }; + } + + private TimeoutException HandleTimeout(Process process, TimeSpan timeout, string executablePath, string arguments) + { + // Get the pre-kill exit state and any kill error (if termination was attempted). + (ExitCheckResult exitCheck, Exception? killException) = process.TryKill(logger); + string command = $"{executablePath} {arguments}"; - process.ErrorDataReceived += (sender, e) => + TimeoutException exception; + + switch (exitCheck.Status) { - if (e.Data == null) - errorWaitHandle.Set(); - else - errorBuilder.AppendLine(e.Data); - }; + case ExitStatus.NotExited: + // Timeout occurred before the process had exited: the process itself exceeded the timeout. + exception = new TimeoutException($"Process execution timed out after {timeout.TotalSeconds} seconds: {command}"); + if (killException is not null) + { + exception.Data["ProcessKillException"] = killException; + logger.LogWarning(killException, "Failed to kill process after timeout for command: {Command}", command); + } + break; + + case ExitStatus.Exited: + // Timeout occurred after the process had already exited, but before streams were fully drained. + exception = new TimeoutException($"Process streams draining timed out after {timeout.TotalSeconds} seconds: {command}"); + break; + + case ExitStatus.Indeterminate: + // Could not determine exit state due to an exception from Process.HasExited (no kill was attempted). + exception = new TimeoutException($"Process execution or streams draining timed out after {timeout.TotalSeconds} seconds: {command}"); + exception.Data["ProcessExitCheckException"] = exitCheck.CheckException; + logger.LogWarning(exitCheck.CheckException, "Could not determine process exit state after the timeout for command: {Command}", command); + break; + + default: + throw new InvalidOperationException($"Unexpected exit status: {exitCheck.Status}"); + } + + exception.Data["ProcessName"] = process.SafeName(); + exception.Data["ProcessId"] = process.SafeId(); + exception.Data["ProcessExitStatus"] = exitCheck.Status.ToString(); - process.Start(); - process.BeginOutputReadLine(); - process.BeginErrorReadLine(); + return exception; + } + + private void HandleCancellation(Process process, string executablePath, string arguments, OperationCanceledException exception) + { + // Get the pre-kill exit state and any kill error (if termination was attempted). + (ExitCheckResult exitCheck, Exception? killException) = process.TryKill(logger); + string command = $"{executablePath} {arguments}"; - var waitTask = Task.WhenAll( - Task.Run(() => process.WaitForExit(timeoutSeconds * 1000), cancellationToken), - Task.Run( - () => + switch (exitCheck.Status) + { + case ExitStatus.NotExited: + // Cancellation occurred before the process had exited. + if (killException is not null) { - outputWaitHandle.WaitOne(1000); - errorWaitHandle.WaitOne(1000); - }, - cancellationToken) - ); + exception.Data["ProcessKillException"] = killException; + logger.LogWarning(killException, "Failed to kill process after cancellation for command: {Command}", command); + } + break; - try + case ExitStatus.Exited: + // Process already exited + break; + + case ExitStatus.Indeterminate: + // Could not determine exit state due to an exception from Process.HasExited (no kill was attempted). + exception.Data["ProcessExitCheckException"] = exitCheck.CheckException; + logger.LogWarning(exitCheck.CheckException, "Could not determine process exit state during cancellation for command: {Command}", command); + break; + + default: + throw new InvalidOperationException($"Unexpected exit status: {exitCheck.Status}"); + } + + exception.Data["ProcessName"] = process.SafeName(); + exception.Data["ProcessId"] = process.SafeId(); + exception.Data["ProcessExitStatus"] = exitCheck.Status.ToString(); + + // we deliberately preserve and rethrow (via 'throw;') the original OCE at call site. + } + + /// + /// Reads either stdout or stderr from a asynchronously. + /// Handlers are attached in the constructor, and reading begins when + /// is called after the process has started. + /// + /// + /// + /// Intended usage pattern: + /// + /// + /// Create and configure the with redirected streams. + /// Construct (handlers attach immediately). + /// Start the process. + /// Call to begin event-driven reading. + /// Await the returned task to obtain the full stream content. + /// + /// + /// + /// The reader does not handle cancellation directly - the underlying + /// owns the reader threads, which naturally terminate when the process exits or is killed. + /// This type: + /// + /// + /// Accumulates received lines into a buffer. + /// Completes when e.Data == null, the signal that the stream closed. + /// + /// Includes a safety-net completion in so callers cannot hang if + /// a close event is never raised (only relevant if used outside ExecuteAsync). + /// + /// + /// + private sealed class ProcessStreamReader : IDisposable + { + private readonly Process _process; + private readonly bool _isErrorStream; + private readonly ILogger _logger; + private readonly StringBuilder _buffer = new(); + private readonly TaskCompletionSource _tcs = new(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly DataReceivedEventHandler _handler; + private bool _readingStarted; + private bool _disposed; + + public ProcessStreamReader(Process process, bool isErrorStream, ILogger logger) { - await waitTask; + this._process = process ?? throw new ArgumentNullException(nameof(process)); + this._isErrorStream = isErrorStream; + _logger = logger ?? throw new ArgumentNullException(nameof(logger)); + + _handler = (_, e) => + { + if (e.Data is null) + { + // Stream closed – finalize accumulated output and complete. + _tcs.TrySetResult(_buffer.ToString()); + } + else + { + _buffer.AppendLine(e.Data); + } + }; + + if (isErrorStream) + { + process.ErrorDataReceived += _handler; + } + else + { + process.OutputDataReceived += _handler; + } } - catch (OperationCanceledException) + + /// + /// Begins asynchronous reading of the associated stream. + /// Must be called only after has successfully completed. + /// + /// + /// This method does not accept a because the underlying + /// BeginOutputReadLine / BeginErrorReadLine APIs are event-driven and do not + /// support token-based cancellation. + /// + /// Cancellation of the overall external process operation is handled by ExecuteAsync, + /// which terminates the process on timeout or external cancellation. Once the process exits + /// (naturally or via kill), the stream read completes automatically. + /// + /// If ProcessStreamReader is ever used outside the normal ExecuteAsync workflow, + /// provides a safety net by completing the task even if the final + /// e.Data == null callback was never raised. + /// + /// A task that completes when the stream has fully drained. + public Task ReadToEndAsync() { - if (!process.HasExited) + ObjectDisposedException.ThrowIf(_disposed, this); + + if (_readingStarted) { - process.Kill(); + throw new InvalidOperationException("StartReading has already been called for this reader."); } - throw; + _readingStarted = true; + + if (_isErrorStream) + { + _process.BeginErrorReadLine(); + } + else + { + _process.BeginOutputReadLine(); + } + + return _tcs.Task; } - if (!process.HasExited) + /// + /// Disposes the stream reader by unsubscribing the data-received handler and ensuring + /// that the task returned by is completed as a safety net. + /// + /// + /// In the normal ExecuteAsync workflow, the read task will already have completed + /// before runs: + /// + /// Success path — the stream has fully drained. + /// Timeout or cancellation — ExecuteAsync has already raised an exception. + /// + /// + /// In these cases, the fallback completion inside is a harmless no-op. + /// Its purpose is to guarantee that callers of never block indefinitely if + /// ProcessStreamReader is used outside the intended ExecuteAsync flow and the + /// final e.Data == null callback is never delivered. + /// + public void Dispose() { - process.Kill(); - throw new TimeoutException($"Process execution timed out after {timeoutSeconds} seconds"); + if (_disposed) + { + return; + } + + _disposed = true; + + try + { + if (_isErrorStream) + { + _process.ErrorDataReceived -= _handler; + } + else + { + _process.OutputDataReceived -= _handler; + } + } + catch (Exception ex) + { + // Unsubscribing the handlers is a best-effort cleanup step; log and swallow any exceptions to avoid disposal throwing. + _logger.LogDebug( + ex, + "Unsubscribe from {StreamType} stream during disposal was skipped. Process: {ProcessName}, PID: {Pid}.", + StreamType, + _process.SafeName(), + _process.SafeId()); + } + + // Safety net: if the DataReceived handler _handler never observed the final e.Data == null + // and therefore never completed the read task, force completion here. + // + // We intentionally avoid calling ToString() on the shared StringBuilder _buffer to prevent + // any cross-thread races with AppendLine() inside the event handler. In all normal + // ExecuteAsync scenarios, the task will already be completed before Dispose() runs, + // making completion here a no-op. + // + // In timeout or cancellation scenarios, the caller receives an exception and does not + // consume stream output, so try completing with an empty string is sufficient and avoids + // touching potentially-mutating state. + if (_tcs.TrySetResult(string.Empty)) + { + _logger.LogDebug( + "ProcessStreamReader for {StreamType} (Process: {ProcessName}, PID: {Pid}) completed during disposal.", + StreamType, + _process.SafeName(), + _process.SafeId()); + } } - return new ProcessResult( - process.ExitCode, - outputBuilder.ToString().TrimEnd(), - errorBuilder.ToString().TrimEnd(), - $"{executablePath} {arguments}"); + private string StreamType => _isErrorStream ? "stderr" : "stdout"; } +} - public JsonElement ParseJsonOutput(ProcessResult result) +/// +/// Extensions for safe process inspection and kill operations. +/// +internal static class ProcessExtensions +{ + public static int SafeId(this Process process) { - if (result.ExitCode != 0) + try { - var error = new ParseError( - result.ExitCode, - result.Error, - result.Command - ); - return JsonSerializer.SerializeToElement(error, ServicesJsonContext.Default.ParseError); + return process.Id; + } + catch + { + return -1; } + } + public static string SafeName(this Process process) + { try { - using var jsonDocument = JsonDocument.Parse(result.Output); - return jsonDocument.RootElement.Clone(); + return process.ProcessName; } catch { - return JsonSerializer.SerializeToElement(new ParseOutput(result.Output), ServicesJsonContext.Default.ParseOutput); + return ""; } } - internal record ParseError( - int ExitCode, - string Error, - string Command - ); - - internal record ParseOutput([property: JsonPropertyName("output")] string Output); - - public void SetEnvironmentVariables(IDictionary variables) + /// + /// Gets the exit state of a process, defensively handling exceptions that may be thrown by . + /// + /// See official docs: + /// https://learn.microsoft.com/dotnet/api/system.diagnostics.process.hasexited + /// + /// + /// The process to check. + /// Logger for diagnostic messages. + /// + /// An indicating: + /// + /// with null exception if the process has exited. + /// with null exception if the process has not exited. + /// with null exception if is thrown. + /// + /// with the exception if or is thrown. + /// + /// + /// + public static ExitCheckResult CheckExitState(this Process process, ILogger logger) { - if (variables == null) + try { - return; + return process.HasExited + ? new ExitCheckResult(ExitStatus.Exited, CheckException: null) + : new ExitCheckResult(ExitStatus.NotExited, CheckException: null); } + catch (InvalidOperationException checkException) + { + // Official docs: "No process is associated with this object." - treat as "already gone". + logger.LogDebug( + checkException, + "Process.HasExited reported no associated process. Treating as already exited. " + + "Process: {ProcessName}, PID: {Pid}", process.SafeName(), process.SafeId()); + return new ExitCheckResult(ExitStatus.Exited, CheckException: null); + } + catch (System.ComponentModel.Win32Exception checkException) + { + // Failure to read status (access denied, invalid handle, etc.). + return new ExitCheckResult(ExitStatus.Indeterminate, checkException); + } + catch (NotSupportedException checkException) + { + // Remote process or unsupported scenario – not a valid case for Azure MCP Server. + return new ExitCheckResult(ExitStatus.Indeterminate, checkException); + } + } - foreach (var pair in variables) + /// + /// Best-effort attempt to terminate the process (and its tree), returning the exit state observed before + /// the kill attempt and any exception from the kill attempt. + /// + /// See official docs: + /// https://learn.microsoft.com/dotnet/api/system.diagnostics.process.Kill + /// + /// + /// The process to terminate. + /// Logger for diagnostic messages. + /// + /// + /// The returned reflects the state observed before the kill attempt. + /// If is reported, this method calls . + /// + /// Kill may throw: + /// + /// + /// – no process is associated with this object (for example, it has + /// already exited or the handle is invalid). In this case, the exception is logged as debug information and + /// is not treated as a kill failure, since there is nothing left to terminate. + /// + /// + /// or + /// – treated as genuine kill failures and returned to the caller for + /// diagnostic purposes. + /// + /// + /// + public static (ExitCheckResult ExitCheck, Exception? KillException) TryKill(this Process process, ILogger logger) + { + var exitCheck = process.CheckExitState(logger); + + if (exitCheck.Status == ExitStatus.NotExited) { - environmentVariables[pair.Key] = pair.Value; + try + { + process.Kill(entireProcessTree: true); + return (exitCheck, null); + } + catch (InvalidOperationException e) + { + // Official docs: "No process is associated with this object." - treat as "already gone". + logger.LogDebug( + e, + "Process.Kill reported no associated process. Treating as already exited. " + + "Process: {ProcessName}, PID: {Pid}", process.SafeName(), process.SafeId()); + + // Considered success from a termination perspective: nothing left to kill. + return (exitCheck, null); + } + catch (System.ComponentModel.Win32Exception e) + { + // Failure to terminate (access denied, invalid handle, etc.). + return (exitCheck, e); + } + catch (NotSupportedException e) + { + // Remote process or unsupported scenario – not a valid case for Azure MCP Server. + return (exitCheck, e); + } } + + // Process has already exited or exit state was indeterminate; nothing to kill. + return (exitCheck, null); + } + + /// + /// Represents the exit status of a process. + /// + public enum ExitStatus + { + /// + /// The process has exited. + /// + Exited, + + /// + /// The process has not exited. + /// + NotExited, + + /// + /// The process exit status could not be determined because + /// threw an exception during the check. + /// + Indeterminate } + + /// + /// Represents the observed exit state of a process as determined by + /// . + /// + /// This captures: + /// + /// + /// + /// The + /// + /// + /// + /// + /// An associated exception only when the status is + /// + /// + /// + /// + public record ExitCheckResult(ExitStatus Status, Exception? CheckException); } diff --git a/core/Azure.Mcp.Core/src/Services/ProcessExecution/IExternalProcessService.cs b/core/Azure.Mcp.Core/src/Services/ProcessExecution/IExternalProcessService.cs index 15a435e404..e4c17a1317 100644 --- a/core/Azure.Mcp.Core/src/Services/ProcessExecution/IExternalProcessService.cs +++ b/core/Azure.Mcp.Core/src/Services/ProcessExecution/IExternalProcessService.cs @@ -12,19 +12,29 @@ public record ProcessResult( public interface IExternalProcessService { /// - /// Executes an external process and returns the result + /// Executes an external process and captures its standard output and error streams. /// - /// Name of the executable to find in PATH or common install locations - /// Arguments to pass to the executable - /// Timeout in seconds - /// Optional additional paths to search for the executable - /// A token to cancel the operation. - /// Process execution result containing exit code, output and error streams + /// Full path to the executable to run. + /// Command-line arguments to pass to the executable. + /// + /// Optional environment variables to set for the process. If null or not provided, no additional + /// environment variables will be set beyond those inherited from the parent process. + /// + /// + /// Total timeout in seconds for the entire operation, including process execution and output capture. + /// Defaults to 300 seconds (5 minutes). Must be greater than zero. + /// + /// + /// Cancellation token to abort the operation. The process will be terminated if cancellation is requested. + /// + /// + /// A containing the exit code, standard output, standard error, and command string. + /// Task ExecuteAsync( - string executableName, + string executablePath, string arguments, - int timeoutSeconds = 300, - IEnumerable? customPaths = null, + IDictionary? environmentVariables = default, + int operationTimeoutSeconds = 300, CancellationToken cancellationToken = default); /// @@ -33,10 +43,4 @@ Task ExecuteAsync( /// Process execution result /// Parsed JSON element or formatted error object if parsing fails JsonElement ParseJsonOutput(ProcessResult result); - - /// - /// Sets environment variables for the process execution - /// /// - /// Dictionary of environment variables to set - void SetEnvironmentVariables(IDictionary variables); } diff --git a/servers/Azure.Mcp.Server/tests/Azure.Mcp.Server.UnitTests/Infrastructure/CommandMetadataSyncTests.cs b/servers/Azure.Mcp.Server/tests/Azure.Mcp.Server.UnitTests/Infrastructure/CommandMetadataSyncTests.cs index 48c00df9c1..94cfaee18d 100644 --- a/servers/Azure.Mcp.Server/tests/Azure.Mcp.Server.UnitTests/Infrastructure/CommandMetadataSyncTests.cs +++ b/servers/Azure.Mcp.Server/tests/Azure.Mcp.Server.UnitTests/Infrastructure/CommandMetadataSyncTests.cs @@ -2,6 +2,7 @@ // Licensed under the MIT License. using Azure.Mcp.Core.Services.ProcessExecution; +using Microsoft.Extensions.Logging.Abstractions; using Xunit; namespace Azure.Mcp.Server.UnitTests.Infrastructure; @@ -31,7 +32,7 @@ public async Task AzCommandsMetadata_Should_Be_Synchronized() var originalContent = File.ReadAllText(docsPath); // Act - Run the update script using ExternalProcessService - var processService = new ExternalProcessService(); + var processService = new ExternalProcessService(NullLogger.Instance); var pwshPath = FindPowerShellExecutable(); var arguments = $"-NoProfile -ExecutionPolicy Bypass -File \"{updateScriptPath}\" -AzmcpPath \"{azmcpPath}\" -DocsPath \"{docsPath}\""; diff --git a/tools/Azure.Mcp.Tools.Extension/src/Commands/AzCommand.cs b/tools/Azure.Mcp.Tools.Extension/src/Commands/AzCommand.cs index 0f1da34a9c..8ef30ea0b9 100644 --- a/tools/Azure.Mcp.Tools.Extension/src/Commands/AzCommand.cs +++ b/tools/Azure.Mcp.Tools.Extension/src/Commands/AzCommand.cs @@ -138,7 +138,9 @@ private async Task AuthenticateWithAzureCredentialsAsync(IExternalProcessS var azPath = FindAzCliPath() ?? throw new FileNotFoundException("Azure CLI executable not found in PATH or common installation locations. Please ensure Azure CLI is installed."); var loginCommand = $"login --service-principal -u {credentials.ClientId} -p {credentials.ClientSecret} --tenant {credentials.TenantId}"; - var result = await processService.ExecuteAsync(azPath, loginCommand, 60, cancellationToken: cancellationToken); + var result = await processService.ExecuteAsync(azPath, loginCommand, + operationTimeoutSeconds: 60, + cancellationToken: cancellationToken); if (result.ExitCode != 0) { @@ -180,7 +182,9 @@ public override async Task ExecuteAsync(CommandContext context, await AuthenticateWithAzureCredentialsAsync(processService, _logger, cancellationToken); var azPath = FindAzCliPath() ?? throw new FileNotFoundException("Azure CLI executable not found in PATH or common installation locations. Please ensure Azure CLI is installed."); - var result = await processService.ExecuteAsync(azPath, command, _processTimeoutSeconds, cancellationToken: cancellationToken); + var result = await processService.ExecuteAsync(azPath, command, + operationTimeoutSeconds: _processTimeoutSeconds, + cancellationToken: cancellationToken); if (result.ExitCode != 0) { diff --git a/tools/Azure.Mcp.Tools.Extension/src/Commands/AzqrCommand.cs b/tools/Azure.Mcp.Tools.Extension/src/Commands/AzqrCommand.cs index 89d30397d7..73ecc8a17d 100644 --- a/tools/Azure.Mcp.Tools.Extension/src/Commands/AzqrCommand.cs +++ b/tools/Azure.Mcp.Tools.Extension/src/Commands/AzqrCommand.cs @@ -103,7 +103,9 @@ public override async Task ExecuteAsync(CommandContext context, command += " --json"; var processService = context.GetService(); - var result = await processService.ExecuteAsync(azqrPath, command, _processTimeoutSeconds, cancellationToken: cancellationToken); + var result = await processService.ExecuteAsync(azqrPath, command, + operationTimeoutSeconds: _processTimeoutSeconds, + cancellationToken: cancellationToken); if (result.ExitCode != 0) { diff --git a/tools/Azure.Mcp.Tools.Extension/tests/Azure.Mcp.Tools.Extension.UnitTests/AzqrCommandTests.cs b/tools/Azure.Mcp.Tools.Extension/tests/Azure.Mcp.Tools.Extension.UnitTests/AzqrCommandTests.cs index b97f9d21e6..3e68e81b00 100644 --- a/tools/Azure.Mcp.Tools.Extension/tests/Azure.Mcp.Tools.Extension.UnitTests/AzqrCommandTests.cs +++ b/tools/Azure.Mcp.Tools.Extension/tests/Azure.Mcp.Tools.Extension.UnitTests/AzqrCommandTests.cs @@ -73,8 +73,8 @@ public async Task ExecuteAsync_ReturnsSuccessResult_WhenScanSucceeds() _processService.ExecuteAsync( Arg.Any(), Arg.Any(), + Arg.Any?>(), Arg.Any(), - Arg.Any>(), Arg.Any()) .Returns(new ProcessResult(0, expectedOutput, string.Empty, $"scan --subscription-id {mockSubscriptionId}")); @@ -90,8 +90,8 @@ public async Task ExecuteAsync_ReturnsSuccessResult_WhenScanSucceeds() await _processService.Received().ExecuteAsync( Arg.Any(), Arg.Any(), + Arg.Any?>(), Arg.Any(), - Arg.Any>(), Arg.Any()); } finally