-
Notifications
You must be signed in to change notification settings - Fork 925
Improve detach mode: fix pipe handle inheritance and unify log naming #14424
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
08c1c4a
b673bda
89af2b6
4883152
9c6d7ad
76d8085
40486d4
6e14e8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |||||||||||||||||||||
| using Aspire.Cli.Configuration; | ||||||||||||||||||||||
| using Aspire.Cli.DotNet; | ||||||||||||||||||||||
| using Aspire.Cli.Interaction; | ||||||||||||||||||||||
| using Aspire.Cli.Processes; | ||||||||||||||||||||||
| using Aspire.Cli.Projects; | ||||||||||||||||||||||
| using Aspire.Cli.Resources; | ||||||||||||||||||||||
| using Aspire.Cli.Telemetry; | ||||||||||||||||||||||
|
|
@@ -86,6 +87,11 @@ internal sealed class RunCommand : BaseCommand | |||||||||||||||||||||
| { | ||||||||||||||||||||||
| Description = RunCommandStrings.NoBuildArgumentDescription | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
| private static readonly Option<string?> s_logFileOption = new("--log-file") | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| Description = "Path to write the log file (used internally by --detach).", | ||||||||||||||||||||||
| Hidden = true | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
| private readonly Option<bool>? _startDebugSessionOption; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| public RunCommand( | ||||||||||||||||||||||
|
|
@@ -126,6 +132,7 @@ public RunCommand( | |||||||||||||||||||||
| Options.Add(s_formatOption); | ||||||||||||||||||||||
| Options.Add(s_isolatedOption); | ||||||||||||||||||||||
| Options.Add(s_noBuildOption); | ||||||||||||||||||||||
| Options.Add(s_logFileOption); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if (ExtensionHelper.IsExtensionHost(InteractionService, out _, out _)) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
|
|
@@ -294,9 +301,9 @@ protected override async Task<int> ExecuteAsync(ParseResult parseResult, Cancell | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Handle remote environments (Codespaces, Remote Containers, SSH) | ||||||||||||||||||||||
| var isCodespaces = dashboardUrls.CodespacesUrlWithLoginToken is not null; | ||||||||||||||||||||||
| var isRemoteContainers = _configuration.GetValue<bool>("REMOTE_CONTAINERS", false); | ||||||||||||||||||||||
| var isSshRemote = _configuration.GetValue<string?>("VSCODE_IPC_HOOK_CLI") is not null | ||||||||||||||||||||||
| && _configuration.GetValue<string?>("SSH_CONNECTION") is not null; | ||||||||||||||||||||||
| var isRemoteContainers = string.Equals(_configuration["REMOTE_CONTAINERS"], "true", StringComparison.OrdinalIgnoreCase); | ||||||||||||||||||||||
| var isSshRemote = _configuration["VSCODE_IPC_HOOK_CLI"] is not null | ||||||||||||||||||||||
| && _configuration["SSH_CONNECTION"] is not null; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| AppendCtrlCMessage(longestLocalizedLengthWithColon); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
@@ -492,7 +499,7 @@ internal static int RenderAppHostSummary( | |||||||||||||||||||||
| new Align(new Markup($"[bold green]{dashboardLabel}[/]:"), HorizontalAlignment.Right), | ||||||||||||||||||||||
| new Markup("[dim]N/A[/]")); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| grid.AddRow(Text.Empty, Text.Empty); | ||||||||||||||||||||||
| grid.AddRow(Text.Empty, Text.Empty); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Logs row | ||||||||||||||||||||||
|
|
@@ -655,18 +662,23 @@ private async Task<int> ExecuteDetachedAsync(ParseResult parseResult, FileInfo? | |||||||||||||||||||||
| _logger.LogDebug("Found {Count} running instance(s) for this AppHost, stopping them first", existingSockets.Length); | ||||||||||||||||||||||
| var manager = new RunningInstanceManager(_logger, _interactionService, _timeProvider); | ||||||||||||||||||||||
| // Stop all running instances in parallel - don't block on failures | ||||||||||||||||||||||
| var stopTasks = existingSockets.Select(socket => | ||||||||||||||||||||||
| var stopTasks = existingSockets.Select(socket => | ||||||||||||||||||||||
| manager.StopRunningInstanceAsync(socket, cancellationToken)); | ||||||||||||||||||||||
| await Task.WhenAll(stopTasks).ConfigureAwait(false); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Build the arguments for the child CLI process | ||||||||||||||||||||||
| // Tell the child where to write its log so we can find it on failure. | ||||||||||||||||||||||
| var childLogFile = GenerateChildLogFilePath(); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| var args = new List<string> | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| "run", | ||||||||||||||||||||||
| "--non-interactive", | ||||||||||||||||||||||
| "--project", | ||||||||||||||||||||||
| effectiveAppHostFile.FullName | ||||||||||||||||||||||
| effectiveAppHostFile.FullName, | ||||||||||||||||||||||
| "--log-file", | ||||||||||||||||||||||
| childLogFile | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Pass through global options that should be forwarded to child CLI | ||||||||||||||||||||||
|
|
@@ -707,29 +719,15 @@ private async Task<int> ExecuteDetachedAsync(ParseResult parseResult, FileInfo? | |||||||||||||||||||||
| dotnetPath, isDotnetHost, string.Join(" ", args)); | ||||||||||||||||||||||
| _logger.LogDebug("Working directory: {WorkingDirectory}", ExecutionContext.WorkingDirectory.FullName); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Redirect stdout/stderr to suppress child output - it writes to log file anyway | ||||||||||||||||||||||
| var startInfo = new ProcessStartInfo | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| FileName = dotnetPath, | ||||||||||||||||||||||
| UseShellExecute = false, | ||||||||||||||||||||||
| CreateNoWindow = true, | ||||||||||||||||||||||
| RedirectStandardOutput = true, | ||||||||||||||||||||||
| RedirectStandardError = true, | ||||||||||||||||||||||
| RedirectStandardInput = false, | ||||||||||||||||||||||
| WorkingDirectory = ExecutionContext.WorkingDirectory.FullName | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // If we're running via `dotnet aspire.dll`, add the DLL as first arg | ||||||||||||||||||||||
| // When running native AOT, don't add the DLL even if it exists in the same folder | ||||||||||||||||||||||
| // Build the full argument list for the child process, including the entry assembly | ||||||||||||||||||||||
| // path when running via `dotnet aspire.dll` | ||||||||||||||||||||||
| var childArgs = new List<string>(); | ||||||||||||||||||||||
| if (isDotnetHost && !string.IsNullOrEmpty(entryAssemblyPath) && entryAssemblyPath.EndsWith(".dll", StringComparison.OrdinalIgnoreCase)) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| startInfo.ArgumentList.Add(entryAssemblyPath); | ||||||||||||||||||||||
| childArgs.Add(entryAssemblyPath); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| foreach (var arg in args) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| startInfo.ArgumentList.Add(arg); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| childArgs.AddRange(args); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Start the child process and wait for the backchannel in a single status spinner | ||||||||||||||||||||||
| Process? childProcess = null; | ||||||||||||||||||||||
|
|
@@ -741,30 +739,10 @@ private async Task<int> ExecuteDetachedAsync(ParseResult parseResult, FileInfo? | |||||||||||||||||||||
| // Failure mode 2: Failed to spawn child process | ||||||||||||||||||||||
| try | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| childProcess = Process.Start(startInfo); | ||||||||||||||||||||||
| if (childProcess is null) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| return null; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Start async reading of stdout/stderr to prevent buffer blocking | ||||||||||||||||||||||
| // Log output for debugging purposes | ||||||||||||||||||||||
| childProcess.OutputDataReceived += (_, e) => | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| if (e.Data is not null) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| _logger.LogDebug("Child stdout: {Line}", e.Data); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
| childProcess.ErrorDataReceived += (_, e) => | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| if (e.Data is not null) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| _logger.LogDebug("Child stderr: {Line}", e.Data); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
| childProcess.BeginOutputReadLine(); | ||||||||||||||||||||||
| childProcess.BeginErrorReadLine(); | ||||||||||||||||||||||
| childProcess = DetachedProcessLauncher.Start( | ||||||||||||||||||||||
| dotnetPath, | ||||||||||||||||||||||
| childArgs, | ||||||||||||||||||||||
| ExecutionContext.WorkingDirectory.FullName); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| catch (Exception ex) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
|
|
@@ -843,10 +821,13 @@ private async Task<int> ExecuteDetachedAsync(ParseResult parseResult, FileInfo? | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| if (childExitedEarly) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| _interactionService.DisplayError(string.Format( | ||||||||||||||||||||||
| CultureInfo.CurrentCulture, | ||||||||||||||||||||||
| RunCommandStrings.AppHostExitedWithCode, | ||||||||||||||||||||||
| childExitCode)); | ||||||||||||||||||||||
| // Show a friendly message based on well-known exit codes from the child | ||||||||||||||||||||||
| var errorMessage = childExitCode switch | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| ExitCodeConstants.FailedToBuildArtifacts => RunCommandStrings.AppHostFailedToBuild, | ||||||||||||||||||||||
| _ => string.Format(CultureInfo.CurrentCulture, RunCommandStrings.AppHostExitedWithCode, childExitCode) | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
| _interactionService.DisplayError(errorMessage); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| else | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
|
|
@@ -866,11 +847,11 @@ private async Task<int> ExecuteDetachedAsync(ParseResult parseResult, FileInfo? | |||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Always show log file path for troubleshooting | ||||||||||||||||||||||
| // Point to the child's log file — it contains the actual build/runtime errors | ||||||||||||||||||||||
| _interactionService.DisplayMessage("magnifying_glass_tilted_right", string.Format( | ||||||||||||||||||||||
| CultureInfo.CurrentCulture, | ||||||||||||||||||||||
| RunCommandStrings.CheckLogsForDetails, | ||||||||||||||||||||||
| _fileLoggerProvider.LogFilePath.EscapeMarkup())); | ||||||||||||||||||||||
| childLogFile.EscapeMarkup())); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| return ExitCodeConstants.FailedToDotnetRunAppHost; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
@@ -890,7 +871,7 @@ private async Task<int> ExecuteDetachedAsync(ParseResult parseResult, FileInfo? | |||||||||||||||||||||
| pid, | ||||||||||||||||||||||
| childProcess.Id, | ||||||||||||||||||||||
| dashboardUrls?.BaseUrlWithLoginToken, | ||||||||||||||||||||||
| _fileLoggerProvider.LogFilePath); | ||||||||||||||||||||||
| childLogFile); | ||||||||||||||||||||||
| var json = JsonSerializer.Serialize(result, RunCommandJsonContext.RelaxedEscaping.DetachOutputInfo); | ||||||||||||||||||||||
| _interactionService.DisplayRawText(json); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
@@ -903,7 +884,7 @@ private async Task<int> ExecuteDetachedAsync(ParseResult parseResult, FileInfo? | |||||||||||||||||||||
| appHostRelativePath, | ||||||||||||||||||||||
| dashboardUrls?.BaseUrlWithLoginToken, | ||||||||||||||||||||||
| codespacesUrl: null, | ||||||||||||||||||||||
| _fileLoggerProvider.LogFilePath, | ||||||||||||||||||||||
| childLogFile, | ||||||||||||||||||||||
| isExtensionHost, | ||||||||||||||||||||||
| pid); | ||||||||||||||||||||||
| _ansiConsole.WriteLine(); | ||||||||||||||||||||||
|
|
@@ -913,4 +894,12 @@ private async Task<int> ExecuteDetachedAsync(ParseResult parseResult, FileInfo? | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| return ExitCodeConstants.Success; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| private string GenerateChildLogFilePath() | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| return Diagnostics.FileLoggerProvider.GenerateLogFilePath( | ||||||||||||||||||||||
| ExecutionContext.LogsDirectory.FullName, | ||||||||||||||||||||||
| _timeProvider, | ||||||||||||||||||||||
| suffix: "detach-child"); | ||||||||||||||||||||||
|
||||||||||||||||||||||
| return Diagnostics.FileLoggerProvider.GenerateLogFilePath( | |
| ExecutionContext.LogsDirectory.FullName, | |
| _timeProvider, | |
| suffix: "detach-child"); | |
| var logsDirectory = ExecutionContext.LogsDirectory.FullName; | |
| var now = _timeProvider.GetUtcNow(); | |
| var timestamp = now.ToString("yyyyMMddHHmmssfff", CultureInfo.InvariantCulture); | |
| var uniqueId = Guid.NewGuid().ToString("N", CultureInfo.InvariantCulture); | |
| var fileName = $"aspire-detach-child-{timestamp}-{uniqueId}.log"; | |
| return Path.Combine(logsDirectory, fileName); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.Diagnostics; | ||
|
|
||
| namespace Aspire.Cli.Processes; | ||
|
|
||
| internal static partial class DetachedProcessLauncher | ||
| { | ||
| /// <summary> | ||
| /// Unix implementation using Process.Start with stdio redirection. | ||
| /// On Linux/macOS, the redirect pipes' original fds are created with O_CLOEXEC, | ||
| /// but dup2 onto fd 0/1/2 clears that flag — so grandchildren DO inherit the pipe | ||
| /// as their stdio. However, since we close the parent's read-end immediately, the | ||
| /// pipe has no reader and writes produce EPIPE (harmless). The key difference from | ||
| /// Windows is that on Unix, only fds 0/1/2 survive exec — no extra handle leakage. | ||
| /// </summary> | ||
| private static Process StartUnix(string fileName, IReadOnlyList<string> arguments, string workingDirectory) | ||
| { | ||
| var startInfo = new ProcessStartInfo | ||
| { | ||
| FileName = fileName, | ||
| UseShellExecute = false, | ||
| CreateNoWindow = true, | ||
| RedirectStandardOutput = true, | ||
| RedirectStandardError = true, | ||
| RedirectStandardInput = false, | ||
| WorkingDirectory = workingDirectory | ||
| }; | ||
|
|
||
| foreach (var arg in arguments) | ||
| { | ||
| startInfo.ArgumentList.Add(arg); | ||
| } | ||
|
|
||
| var process = Process.Start(startInfo) | ||
| ?? throw new InvalidOperationException("Failed to start detached process"); | ||
|
|
||
| // Close the parent's read-end of the pipes. This means the pipe has no reader, | ||
| // so if the grandchild (AppHost) writes to inherited stdout/stderr, it gets EPIPE | ||
| // which is harmless. The important thing is no caller is blocked waiting on the | ||
| // pipe — unlike Windows where the handle stays open and blocks execSync callers. | ||
| process.StandardOutput.Close(); | ||
| process.StandardError.Close(); | ||
|
|
||
| return process; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes to detached execution introduced new behaviors that are currently untested (e.g.,
--log-filepropagation, exit-code-to-message mapping forFailedToBuildArtifacts, and ensuring the correct log file path is surfaced on failure). There are existingRunCommandTests, but none cover--detach; adding focused unit/integration tests for these paths would help prevent regressions on Windows and in programmatic consumers.