Improve detach mode: fix pipe handle inheritance and unify log naming#14424
Improve detach mode: fix pipe handle inheritance and unify log naming#14424davidfowl merged 8 commits intorelease/13.2from
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 14424Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 14424" |
There was a problem hiding this comment.
Pull request overview
This PR improves aspire run --detach behavior by preventing pipe/handle inheritance issues (notably on Windows) and standardizing how CLI log files are named and discovered, so detached failures can reliably point users to the right diagnostics.
Changes:
- Adds a hidden
--log-fileoption and passes it from the detach parent process to the detach child process. - Introduces
FileLoggerProvider.GenerateLogFilePath(...)and updates log file naming tocli_yyyyMMddTHHmmss_{pid}[_{suffix}].log. - Improves detached-mode failure UX by mapping
FailedToBuildArtifactsto a friendlier localized message and pointing to the child’s log file.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Aspire.Cli/Program.cs | Parses --log-file early and uses it to initialize file logging before host build. |
| src/Aspire.Cli/Diagnostics/FileLoggerProvider.cs | Adds centralized log filename generation and updates default naming format. |
| src/Aspire.Cli/Commands/RunCommand.cs | Adds hidden --log-file, stops redirecting detached child stdout/stderr, passes explicit child log path, and improves failure messaging. |
| src/Aspire.Cli/Commands/CacheCommand.cs | Updates log filename suffix matching to align with new cli_{timestamp}_{pid}.log format. |
| src/Aspire.Cli/Resources/RunCommandStrings.resx | Adds new AppHostFailedToBuild resource string. |
| src/Aspire.Cli/Resources/RunCommandStrings.Designer.cs | Regenerates strongly-typed accessor for AppHostFailedToBuild. |
| src/Aspire.Cli/Resources/xlf/RunCommandStrings.cs.xlf | Adds AppHostFailedToBuild localization entry (new). |
| src/Aspire.Cli/Resources/xlf/RunCommandStrings.de.xlf | Adds AppHostFailedToBuild localization entry (new). |
| src/Aspire.Cli/Resources/xlf/RunCommandStrings.es.xlf | Adds AppHostFailedToBuild localization entry (new). |
| src/Aspire.Cli/Resources/xlf/RunCommandStrings.fr.xlf | Adds AppHostFailedToBuild localization entry (new). |
| src/Aspire.Cli/Resources/xlf/RunCommandStrings.it.xlf | Adds AppHostFailedToBuild localization entry (new). |
| src/Aspire.Cli/Resources/xlf/RunCommandStrings.ja.xlf | Adds AppHostFailedToBuild localization entry (new). |
| src/Aspire.Cli/Resources/xlf/RunCommandStrings.ko.xlf | Adds AppHostFailedToBuild localization entry (new). |
| src/Aspire.Cli/Resources/xlf/RunCommandStrings.pl.xlf | Adds AppHostFailedToBuild localization entry (new). |
| src/Aspire.Cli/Resources/xlf/RunCommandStrings.pt-BR.xlf | Adds AppHostFailedToBuild localization entry (new). |
| src/Aspire.Cli/Resources/xlf/RunCommandStrings.ru.xlf | Adds AppHostFailedToBuild localization entry (new). |
| src/Aspire.Cli/Resources/xlf/RunCommandStrings.tr.xlf | Adds AppHostFailedToBuild localization entry (new). |
| src/Aspire.Cli/Resources/xlf/RunCommandStrings.zh-Hans.xlf | Adds AppHostFailedToBuild localization entry (new). |
| src/Aspire.Cli/Resources/xlf/RunCommandStrings.zh-Hant.xlf | Adds AppHostFailedToBuild localization entry (new). |
Files not reviewed (1)
- src/Aspire.Cli/Resources/RunCommandStrings.Designer.cs: Language not supported
Comments suppressed due to low confidence (2)
src/Aspire.Cli/Commands/CacheCommand.cs:121
cache clearskips deleting the current log file by checking a filename suffix based onEnvironment.ProcessId, but the CLI can now run with an explicit--log-filepath (and detached child logs also include an extra suffix). This can cause the current session’s log file to be deleted while still in use. Consider skipping deletion by comparing againstExecutionContext.LogFilePath(full path) instead of relying on the filename pattern.
// Also clear the logs directory (skip current process's log file)
var logsDirectory = ExecutionContext.LogsDirectory;
// Log files are named cli_{timestamp}_{pid}.log, so we need to check the suffix
var currentLogFileSuffix = $"_{Environment.ProcessId}.log";
if (logsDirectory.Exists)
{
foreach (var file in logsDirectory.GetFiles("*", SearchOption.AllDirectories))
{
// Skip the current process's log file to avoid deleting it while in use
if (file.Name.EndsWith(currentLogFileSuffix, StringComparison.OrdinalIgnoreCase))
{
src/Aspire.Cli/Commands/RunCommand.cs:665
- Detached mode now generates/passes a
childLogFileand (on failure) points users to that file, but the success paths still report_fileLoggerProvider.LogFilePath(parent session log) in JSON output and the rendered summary. Since the child process is the one that continues running and writing logs, consider returning/displaying the child log file path consistently to avoid sending users/tools to a log that won’t contain the relevant AppHost build/runtime output.
// 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,
"--log-file",
childLogFile
};
| return Diagnostics.FileLoggerProvider.GenerateLogFilePath( | ||
| ExecutionContext.LogsDirectory.FullName, | ||
| _timeProvider, | ||
| suffix: "detach-child"); |
There was a problem hiding this comment.
GenerateChildLogFilePath() calls FileLoggerProvider.GenerateLogFilePath(), which embeds Environment.ProcessId. Since this method runs in the parent process, the child log file name ends up containing the parent PID, which is misleading and breaks the “pid in filename == process writing the log” convention used elsewhere. Consider adjusting the helper to accept an explicit PID (or use a different uniqueness token for precomputed paths) so detached child log files can reflect the child process ID.
This issue also appears on line 653 of the same file.
| 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); |
| // 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); |
There was a problem hiding this comment.
Changes to detached execution introduced new behaviors that are currently untested (e.g., --log-file propagation, exit-code-to-message mapping for FailedToBuildArtifacts, and ensuring the correct log file path is surfaced on failure). There are existing RunCommandTests, but none cover --detach; adding focused unit/integration tests for these paths would help prevent regressions on Windows and in programmatic consumers.
🎬 CLI E2E Test RecordingsThe following terminal recordings are available for commit
📹 Recordings uploaded automatically from CI run #21959085241 |
f98890f to
ba11a15
Compare
ba11a15 to
08c1c4a
Compare
The previous approach (RedirectStandardOutput=true + close streams) still created inheritable pipe handles. Instead, keep Redirect=false to avoid pipe inheritance and have the child process suppress its own console output when --log-file is specified (the signal that it's a detach child).
Replace Process.Start with platform-specific launcher that suppresses child output and prevents handle/fd inheritance to grandchildren: - Windows: P/Invoke CreateProcess with STARTUPINFOEX and PROC_THREAD_ATTRIBUTE_HANDLE_LIST (same approach as Docker/hcsshim). Child stdout/stderr go to NUL, only NUL handle is inheritable. - Linux/macOS: Process.Start with RedirectStdout=true + close streams. Pipes are O_CLOEXEC so grandchild never inherits them. Removes the Console.SetOut(TextWriter.Null) workaround from Program.cs.
dup2 onto fd 0/1/2 clears O_CLOEXEC, so grandchildren DO inherit the pipe as their stdio. With the parent's read-end closed, writes produce harmless EPIPE. Updated comments to accurately describe the Unix fd inheritance model based on dotnet/runtime pal_process.c source.
09fbf4c to
40486d4
Compare
| } | ||
|
|
||
| // Close the process and thread handles — we only need the PID | ||
| CloseHandle(pi.hProcess); |
There was a problem hiding this comment.
Should this be done after calling Process.GetProcessById(pi.dwProcessId);?
|
Found one behavioral regression to fix before merge:
This can silently redirect CLI logging to an unintended path and violate normal option-boundary semantics. Suggested fix: stop scanning once |
Description
When running
aspire run --detach, the child CLI process inherited pipe handles from stdout/stderr redirection. This kept pipes open until the AppHost grandchild died, preventing callers using synchronous process APIs (e.g. Node.jsexecSync) from detecting that the CLI had exited. This is primarily a Windows issue due to how handle inheritance works.Changes
--log-filehidden option — parent passes an explicit log file path to the child so the parent knows where to point on failure.FileLoggerProvider.GenerateLogFilePathstatic helper used by both the normal logger and the detach child path. Format:cli_20260210T074038_12345.log(with optional suffix).FailedToBuildArtifactsexit code to a friendly message. Points to the child's log file on failure.Checklist